| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZpDSz5dp+zFPYjVw@yilunxu-OptiPlex-7050>
Date: Fri, 12 Jul 2024 14:53:03 +0800
From: Xu Yilun <yilun.xu@...ux.intel.com>
To: iansdannapel@...il.com
Cc: Moritz Fischer <mdf@...nel.org>, Wu Hao <hao.wu@...el.com>,
Xu Yilun <yilun.xu@...el.com>, Tom Rix <trix@...hat.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Heiko Stuebner <heiko.stuebner@...rry.de>,
Neil Armstrong <neil.armstrong@...aro.org>,
Sebastian Reichel <sre@...nel.org>,
Chris Morgan <macromorgan@...mail.com>,
Michael Riesch <michael.riesch@...fvision.net>,
Rafał Miłecki <rafal@...ecki.pl>,
Andre Przywara <andre.przywara@....com>,
Linus Walleij <linus.walleij@...aro.org>,
linux-fpga@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] fpga: Add Efinix Trion & Titanium serial SPI
programming driver
On Fri, Jun 28, 2024 at 05:23:46PM +0200, iansdannapel@...il.com wrote:
> From: Ian Dannapel <iansdannapel@...il.com>
>
Please don't reply to the previous series when you post a new version.
> Add a new driver for loading binary firmware using "SPI passive
Loading to some nvram or reporgraming to FPGA logic blocks.
> programming" on Efinix FPGAs.
>
> Signed-off-by: Ian Dannapel <iansdannapel@...il.com>
> ---
> drivers/fpga/Kconfig | 8 +
> drivers/fpga/Makefile | 1 +
> drivers/fpga/efinix-trion-spi-passive.c | 219 ++++++++++++++++++++++++
> 3 files changed, 228 insertions(+)
> create mode 100644 drivers/fpga/efinix-trion-spi-passive.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 37b35f58f0df..25579510e49e 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -83,6 +83,14 @@ config FPGA_MGR_XILINX_SPI
> FPGA manager driver support for Xilinx FPGA configuration
> over slave serial interface.
>
> +config FPGA_MGR_EFINIX_SPI
> + tristate "Efinix FPGA configuration over SPI passive"
> + depends on SPI
> + help
> + This option enables support for the FPGA manager driver to
> + configure Efinix Trion and Titanium Series FPGAs over SPI
> + using passive serial mode.
> +
> config FPGA_MGR_ICE40_SPI
> tristate "Lattice iCE40 SPI"
> depends on OF && SPI
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index aeb89bb13517..1a95124ff847 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_FPGA_MGR_TS73XX) += ts73xx-fpga.o
> obj-$(CONFIG_FPGA_MGR_XILINX_CORE) += xilinx-core.o
> obj-$(CONFIG_FPGA_MGR_XILINX_SELECTMAP) += xilinx-selectmap.o
> obj-$(CONFIG_FPGA_MGR_XILINX_SPI) += xilinx-spi.o
> +obj-$(CONFIG_FPGA_MGR_EFINIX_SPI) += efinix-trion-spi-passive.o
> obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += zynqmp-fpga.o
> obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA) += versal-fpga.o
> diff --git a/drivers/fpga/efinix-trion-spi-passive.c b/drivers/fpga/efinix-trion-spi-passive.c
> new file mode 100644
> index 000000000000..eb2592e788b9
> --- /dev/null
> +++ b/drivers/fpga/efinix-trion-spi-passive.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Trion and Titanium Series FPGA SPI Passive Programming Driver
> + *
> + * Copyright (C) 2024 iris-GmbH infrared & intelligent sensors
> + *
> + * Ian Dannapel <iansdannapel@...il.com>
> + *
> + * Manage Efinix FPGA firmware that is loaded over SPI using
> + * the serial configuration interface.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sizes.h>
> +
> +struct efinix_spi_conf {
> + struct spi_device *spi;
> + struct gpio_desc *cdone;
> + struct gpio_desc *creset;
> + struct gpio_desc *cs;
> +};
> +
> +static int get_cdone_gpio(struct fpga_manager *mgr)
Is it better use 'struct efinix_spi_conf *conf' as parameter?
Same for the following functions.
> +{
> + struct efinix_spi_conf *conf = mgr->priv;
> + int ret;
> +
> + ret = gpiod_get_value(conf->cdone);
> + if (ret < 0)
> + dev_err(&mgr->dev, "Error reading CDONE (%d)\n", ret);
> +
> + return ret;
> +}
> +
> +static void reset(struct fpga_manager *mgr)
Please unify the naming of the internal functions. You use
'efinix_spi_apply_clk_cycles()' below.
> +{
> + struct efinix_spi_conf *conf = mgr->priv;
> +
> + gpiod_set_value(conf->creset, 1);
> + /* wait tCRESET_N */
> + usleep_range(5, 15);
> + gpiod_set_value(conf->creset, 0);
> +}
> +
> +static enum fpga_mgr_states efinix_spi_state(struct fpga_manager *mgr)
> +{
> + struct efinix_spi_conf *conf = mgr->priv;
> +
> + if (conf->cdone && get_cdone_gpio(mgr) == 1)
> + return FPGA_MGR_STATE_OPERATING;
> +
> + return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int efinix_spi_apply_clk_cycles(struct fpga_manager *mgr)
> +{
> + struct efinix_spi_conf *conf = mgr->priv;
> + char data[13] = {0};
> +
> + return spi_write(conf->spi, data, sizeof(data));
> +}
> +
> +static int efinix_spi_write_init(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + struct efinix_spi_conf *conf = mgr->priv;
> +
> + if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> + dev_err(&mgr->dev, "Partial reconfiguration not supported\n");
> + return -EINVAL;
> + }
> +
> + /* reset with chip select active */
> + gpiod_set_value(conf->cs, 1);
Why operating chip selective at SPI client driver? Isn't it the job for SPI
controller?
> + usleep_range(5, 15);
> + reset(mgr);
> +
> + /* wait tDMIN */
> + usleep_range(100, 150);
> +
> + return 0;
> +}
> +
> +static int efinix_spi_write(struct fpga_manager *mgr, const char *buf,
> + size_t count)
> +{
> + struct efinix_spi_conf *conf = mgr->priv;
> + int ret;
> +
> + ret = spi_write(conf->spi, buf, count);
> + if (ret) {
> + dev_err(&mgr->dev, "SPI error in firmware write: %d\n",
> + ret);
> + return ret;
> + }
> +
> + /* append at least 100 clock cycles */
> + efinix_spi_apply_clk_cycles(mgr);
> +
> + /* release chip select */
> + gpiod_set_value(conf->cs, 0);
Is it correct? What if there is remaining data to write?
> +
> + return 0;
> +}
> +
> +static int efinix_spi_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + struct efinix_spi_conf *conf = mgr->priv;
> + unsigned long timeout =
> + jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
> + bool expired = false;
> + int done;
> +
> + if (conf->cdone) {
> + while (!expired) {
> + expired = time_after(jiffies, timeout);
> +
> + done = get_cdone_gpio(mgr);
> + if (done < 0)
> + return done;
> +
> + if (done)
> + break;
> + }
> + }
> +
> + if (expired)
> + return -ETIMEDOUT;
> +
> + /* wait tUSER */
> + usleep_range(75, 125);
> +
> + return 0;
> +}
> +
> +static const struct fpga_manager_ops efinix_spi_ops = {
> + .state = efinix_spi_state,
> + .write_init = efinix_spi_write_init,
> + .write = efinix_spi_write,
> + .write_complete = efinix_spi_write_complete,
> +};
> +
> +static int efinix_spi_probe(struct spi_device *spi)
> +{
> + struct efinix_spi_conf *conf;
> + struct fpga_manager *mgr;
> +
> + conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
> + if (!conf)
> + return -ENOMEM;
> +
> + conf->spi = spi;
> +
> + conf->creset = devm_gpiod_get(&spi->dev, "creset", GPIOD_OUT_HIGH);
> + if (IS_ERR(conf->creset))
> + return dev_err_probe(&spi->dev, PTR_ERR(conf->creset),
> + "Failed to get RESET gpio\n");
> +
> + conf->cs = devm_gpiod_get(&spi->dev, "cs", GPIOD_OUT_HIGH);
> + if (IS_ERR(conf->cs))
> + return dev_err_probe(&spi->dev, PTR_ERR(conf->cs),
> + "Failed to get CHIP_SELECT gpio\n");
> +
> + if (!(spi->mode & SPI_CPHA) || !(spi->mode & SPI_CPOL))
> + return dev_err_probe(&spi->dev, PTR_ERR(conf->cs),
> + "Unsupported SPI mode, set CPHA and CPOL\n");
> +
> + conf->cdone = devm_gpiod_get_optional(&spi->dev, "cdone", GPIOD_IN);
> + if (IS_ERR(conf->cdone))
> + return dev_err_probe(&spi->dev, PTR_ERR(conf->cdone),
> + "Failed to get CDONE gpio\n");
> +
> + mgr = devm_fpga_mgr_register(&spi->dev,
> + "Efinix SPI Passive Programming FPGA Manager",
> + &efinix_spi_ops, conf);
> +
> + return PTR_ERR_OR_ZERO(mgr);
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id efnx_spi_of_match[] = {
> + { .compatible = "efinix,trion-spi-passive", },
> + { .compatible = "efinix,titanium-spi-passive", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, efnx_spi_of_match);
> +#endif
> +
> +static const struct spi_device_id efinix_ids[] = {
> + { "trion-spi-passive", 0 },
> + { "titanium-spi-passive", 0 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(spi, efinix_ids);
> +
> +
remove the extra blank line.
> +static struct spi_driver efinix_spi_passive_driver = {
> + .driver = {
> + .name = "efinix-fpga-spi-passive",
> + .of_match_table = of_match_ptr(efnx_spi_of_match),
Is it OK remove CONFIG_OF & of_match_ptr()?
Thanks,
Yilun
> + },
> + .probe = efinix_spi_probe,
> + .id_table = efinix_ids,
> +};
> +
> +module_spi_driver(efinix_spi_passive_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Ian Dannapel <iansdannapel@...il.com>");
> +MODULE_DESCRIPTION("Load Efinix FPGA firmware over SPI passive");
> --
> 2.34.1
>
>
Powered by blists - more mailing lists