Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754579AbcLTTrp (ORCPT ); Tue, 20 Dec 2016 14:47:45 -0500 Received: from mail-pf0-f196.google.com ([209.85.192.196]:33629 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752803AbcLTTrl (ORCPT ); Tue, 20 Dec 2016 14:47:41 -0500 Subject: Re: [PATCH v6 4/5] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <20161219072326.fael3uughtghexl4@pengutronix.de> Cc: Alan Tull , Moritz Fischer , Russell King , Catalin Marinas , Will Deacon , Shawn Guo , Sascha Hauer , Fabio Estevam , Mark Rutland , devicetree@vger.kernel.org, linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org, Rob Herring , Anatolij Gustschin , linux-arm-kernel@lists.infradead.org From: Joshua Clayton Message-ID: Date: Tue, 20 Dec 2016 11:47:37 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161219072326.fael3uughtghexl4@pengutronix.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9947 Lines: 295 Uwe, Thanks so much for your review. On 12/18/2016 11:23 PM, Uwe Kleine-K?nig wrote: > On Fri, Dec 16, 2016 at 03:17:53PM -0800, Joshua Clayton wrote: >> cyclone-ps-spi loads FPGA firmware over spi, using the "passive serial" >> interface on Altera Cyclone FPGAS. >> >> This is one of the simpler ways to set up an FPGA at runtime. >> The signal interface is close to unidirectional spi with lsb first. >> >> Signed-off-by: Joshua Clayton >> --- >> drivers/fpga/Kconfig | 7 ++ >> drivers/fpga/Makefile | 1 + >> drivers/fpga/cyclone-ps-spi.c | 186 ++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 194 insertions(+) >> create mode 100644 drivers/fpga/cyclone-ps-spi.c >> >> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig >> index ce861a2..e6c032d 100644 >> --- a/drivers/fpga/Kconfig >> +++ b/drivers/fpga/Kconfig >> @@ -20,6 +20,13 @@ config FPGA_REGION >> FPGA Regions allow loading FPGA images under control of >> the Device Tree. >> >> +config FPGA_MGR_CYCLONE_PS_SPI >> + tristate "Altera Cyclone FPGA Passive Serial over SPI" >> + depends on SPI >> + help >> + FPGA manager driver support for Altera Cyclone using the >> + passive serial interface over SPI >> + >> config FPGA_MGR_SOCFPGA >> tristate "Altera SOCFPGA FPGA Manager" >> depends on ARCH_SOCFPGA || COMPILE_TEST >> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile >> index 8df07bc..a112bef 100644 >> --- a/drivers/fpga/Makefile >> +++ b/drivers/fpga/Makefile >> @@ -6,6 +6,7 @@ >> obj-$(CONFIG_FPGA) += fpga-mgr.o >> >> # FPGA Manager Drivers >> +obj-$(CONFIG_FPGA_MGR_CYCLONE_PS_SPI) += cyclone-ps-spi.o >> obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o >> obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10) += socfpga-a10.o >> obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o >> diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c >> new file mode 100644 >> index 0000000..f9126f9 >> --- /dev/null >> +++ b/drivers/fpga/cyclone-ps-spi.c >> @@ -0,0 +1,186 @@ >> +/** >> + * Altera Cyclone Passive Serial SPI Driver >> + * >> + * Copyright (c) 2017 United Western Technologies, Corporation > In which timezone it's already 2017? s/ / / > LOL. It will be 2017 long before 4.11 was my thinking. I guess I've never spent much time on time stamp etiquette for copyright. It said "2015" in v5, despite still being revised. >> + * >> + * Joshua Clayton >> + * >> + * Manage Altera FPGA firmware that is loaded over spi using the passive >> + * serial configuration method. >> + * Firmware must be in binary "rbf" format. >> + * Works on Cyclone V. Should work on cyclone series. >> + * May work on other Altera FPGAs. > I can test this later on an Arria 10. I'm not sure what the connection > between "Cyclone" and "Arria" is, but the protocol looks similar. My guess was it would be. Would be wonderful to have someone to test. >> + * >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define FPGA_RESET_TIME 50 /* time in usecs to trigger FPGA config */ >> +#define FPGA_MIN_DELAY 50 /* min usecs to wait for config status */ >> +#define FPGA_MAX_DELAY 1000 /* max usecs to wait for config status */ >> + >> +struct cyclonespi_conf { >> + struct gpio_desc *config; >> + struct gpio_desc *status; >> + struct spi_device *spi; >> +}; >> + >> +static const struct of_device_id of_ef_match[] = { >> + { .compatible = "altr,cyclone-ps-spi-fpga-mgr", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, of_ef_match); > barebox already has such a driver, the binding is available at > > https://git.pengutronix.de/cgit/barebox/tree/Documentation/devicetree/bindings/firmware/altr,passive-serial.txt > > (This isn't completely accurate because nstat is optional in the driver.) Interesting. The binding looks ... like we should synchronize those bindings. In the case of my hardware, I don't have access to the confd, but do have access to nstat. I was thinking that using confd to signal done would be a nice but optional... Ideally you'd have both. >> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr) >> +{ >> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; >> + >> + if (gpiod_get_value(conf->status)) >> + return FPGA_MGR_STATE_RESET; >> + >> + return FPGA_MGR_STATE_UNKNOWN; >> +} >> + >> +static int cyclonespi_write_init(struct fpga_manager *mgr, >> + struct fpga_image_info *info, >> + const char *buf, size_t count) >> +{ >> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; >> + int i; >> + >> + if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) { >> + dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); >> + return -EINVAL; >> + } >> + >> + gpiod_set_value(conf->config, 1); >> + usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20); >> + if (!gpiod_get_value(conf->status)) { >> + dev_err(&mgr->dev, "Status pin should be low.\n"); > You write this when get_value returns 0. There is something fishy. I'll take a look. These gpios are "active low", so a logical 1 is a physical low, IIRC. Maybe I should change the wording: Something like dev_err(&mgr->dev, "Status pin should be in reset.\n"); Perhaps? >> + return -EIO; >> + } >> + >> + gpiod_set_value(conf->config, 0); >> + for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) { >> + usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20); >> + if (!gpiod_get_value(conf->status)) >> + return 0; >> + } >> + >> + dev_err(&mgr->dev, "Status pin not ready.\n"); >> + return -EIO; > For Arria 10 the documentation has: > > To ensure a successful configuration, send the entire > configuration data to the device. CONF_DONE is released high > when the device receives all the configuration data > successfully. After CONF_DONE goes high, send two additional > falling edges on DCLK to begin initialization and enter user > mode. > > ISTR this is necessary for Arria V, too. DCLK is the spi clock, yes? Would sending an extra byte after CONF_DONE is released suffice? >> +} >> + >> +static void rev_buf(void *buf, size_t len) >> +{ >> + u32 *fw32 = (u32 *)buf; >> + const u32 *fw_end = (u32 *)(buf + len); >> + >> + /* set buffer to lsb first */ >> + while (fw32 < fw_end) { >> + *fw32 = bitrev8x4(*fw32); >> + fw32++; >> + } > Is the size of the firmware always a multiple of 32 bit? If len isn't a > multiple of 4 you access data after the end of buf. The rbf cyclone V bitstream is padded out with extra bytes of FFFFFFFF and always a multiple of 4 bytes. I could not find anywhere this is documented. I guess we should not assume this will always be the case. I'll add something to handle the tail. >> +} >> + >> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf, >> + size_t count) >> +{ >> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; >> + const char *fw_data = buf; >> + const char *fw_data_end = fw_data + count; >> + >> + while (fw_data < fw_data_end) { >> + int ret; >> + size_t stride = min(fw_data_end - fw_data, SZ_4K); >> + >> + rev_buf((void *)fw_data, stride); > This isn't necessary if the spi controller supports SPI_LSB_FIRST. At > least the mvebu spi core can do this for you. (The driver doesn't > support it yet, though.) This is true, but many of them do not. Moritz Fischer had proposal to add things like this with a flag. It could then be part of the library, rather than part of the driver Speaking of which, I made an unsuccessful attempt to hack generic lsb first SPI with an extra bounce buffer. Sending was fine, but I ran into trouble with LSB first rx (I think) because of dma. >> + ret = spi_write(conf->spi, fw_data, stride); >> + if (ret) { >> + dev_err(&mgr->dev, "spi error in firmware write: %d\n", >> + ret); >> + return ret; >> + } >> + fw_data += stride; >> + } >> + >> + return 0; >> +} >> [...] >> +static int cyclonespi_probe(struct spi_device *spi) >> +{ >> + struct cyclonespi_conf *conf = devm_kzalloc(&spi->dev, sizeof(*conf), >> + GFP_KERNEL); > please indent to the opening (. Will fix. >> + >> + if (!conf) >> + return -ENOMEM; >> + >> + conf->spi = spi; >> + conf->config = devm_gpiod_get(&spi->dev, "config", GPIOD_OUT_HIGH); >> + if (IS_ERR(conf->config)) { >> + dev_err(&spi->dev, "Failed to get config gpio: %ld\n", >> + PTR_ERR(conf->config)); >> + return PTR_ERR(conf->config); >> + } >> + >> + conf->status = devm_gpiod_get(&spi->dev, "status", GPIOD_IN); >> + if (IS_ERR(conf->status)) { >> + dev_err(&spi->dev, "Failed to get status gpio: %ld\n", >> + PTR_ERR(conf->status)); >> + return PTR_ERR(conf->status); >> + } >> + >> + return fpga_mgr_register(&spi->dev, >> + "Altera Cyclone PS SPI FPGA Manager", >> + &cyclonespi_ops, conf); >> +} >> + >> +static int cyclonespi_remove(struct spi_device *spi) >> +{ >> + fpga_mgr_unregister(&spi->dev); >> + >> + return 0; >> +} >> + >> +static struct spi_driver cyclonespi_driver = { >> + .driver = { >> + .name = "cyclone-ps-spi", >> + .owner = THIS_MODULE, >> + .of_match_table = of_match_ptr(of_ef_match), >> + }, >> + .probe = cyclonespi_probe, >> + .remove = cyclonespi_remove, >> +}; > I'm not a big fan of aligning the assignment operators. This tends to > get out of sync or results in bigger than necessary changes in follow up > patches. Note that it's out of sync already now, so I suggest to use a > single space before =. Yes, I can see it your way. Will change. >> + >> +module_spi_driver(cyclonespi_driver) >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_AUTHOR("Joshua Clayton "); >> +MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi"); >> -- > Best regards > Uwe > Even bester regards, Joshua