Message-ID: <8257d6b1-5c15-4e3c-93a7-71ab5d6cb19b@redhat.com>
Date: Thu, 20 Jun 2024 20:05:22 +0200
From: Marco Pagani <marpagan@...hat.com>
To: iansdannapel@...il.com
Cc: mdf@...nel.org, hao.wu@...el.com, yilun.xu@...el.com, trix@...hat.com,
 linux-kernel@...r.kernel.org, linux-fpga@...r.kernel.org
Subject: Re: [PATCH 1/3] fpga: Add Efinix Trion & Titanium serial SPI
 programming driver



On 2024-06-20 16:42, iansdannapel@...il.com wrote:
> From: Ian Dannapel <iansdannapel@...il.com>
> 
> Add a new driver for loading binary firmware using "SPI passive
> programming" on Efinix FPGAs.
> 
> Signed-off-by: Ian Dannapel <iansdannapel@...il.com>
> ---
>  drivers/fpga/Kconfig      |   8 ++
>  drivers/fpga/Makefile     |   1 +
>  drivers/fpga/efinix-spi.c | 219 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+)
>  create mode 100644 drivers/fpga/efinix-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 37b35f58f0df..cb3a6628fa71 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..adbd51d2cd1e 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-spi.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-spi.c b/drivers/fpga/efinix-spi.c
> new file mode 100644
> index 000000000000..7f7d7e6714ae
> --- /dev/null
> +++ b/drivers/fpga/efinix-spi.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 *done;
> +	struct gpio_desc *reset;
> +	struct gpio_desc *cs;
> +	enum fpga_mgr_states state;

This is a bit confusing. I wouldn't use an fpga_mgr_states enum in the
context struct of the low-level module since they define the state of
the fpga manager core. If possible, I would have read the physical
state of the device in the state op to determine if the fpga is
already programmed or in an unknown or error state, since the op is
called only during device registration to set the initial state.

Also, a quick check with checkpatch.pl returned a couple of style
issues.

> +};
> +
> +static int get_done_gpio(struct fpga_manager *mgr)
> +{
> +	struct efinix_spi_conf *conf = mgr->priv;
> +	int ret = 0;
> +
> +	if (conf->done) {
> +		ret = gpiod_get_value(conf->done);
> +		if (ret < 0)
> +			dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static void reset(struct fpga_manager *mgr)
> +{
> +	struct efinix_spi_conf *conf = mgr->priv;
> +
> +	gpiod_set_value(conf->reset, 1);
> +	/* wait tCRESET_N */
> +	usleep_range(5, 15);
> +	gpiod_set_value(conf->reset, 0);
> +	conf->state = FPGA_MGR_STATE_RESET;
> +}
> +
> +static enum fpga_mgr_states efinix_spi_state(struct fpga_manager *mgr)
> +{
> +	struct efinix_spi_conf *conf = mgr->priv;
> +
> +	return conf->state;
> +}
> +
> +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);
> +	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);
> +
> +	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->done) {
> +		while (!expired) {
> +			expired = time_after(jiffies, timeout);
> +
> +			done = get_done_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->state = FPGA_MGR_STATE_UNKNOWN;
> +
> +	conf->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(conf->reset))
> +		return dev_err_probe(&spi->dev, PTR_ERR(conf->reset),
> +				"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->done = devm_gpiod_get_optional(&spi->dev, "done", GPIOD_IN);

I'm not familiar with this FPGA, but from the code, it seems to me that
you also want to support the case where a "done" line is not available.
In such a case, what happens if you start a new programming cycle before
waiting for the previous one to complete? Also, checking if (conf->done)
in get_done_gpio() seems to be redundant since the function is called only
in efinix_spi_write_complete() after checking if (conf->done).


> +	if (IS_ERR(conf->done))
> +		return dev_err_probe(&spi->dev, PTR_ERR(conf->done),
> +				"Failed to get DONE 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 = "efnx,fpga-spi-passive", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, efnx_spi_of_match);
> +#endif
> +
> +static const struct spi_device_id efinix_ids[] = {
> +	{ "fpga-spi-passive", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(spi, efinix_ids);
> +
> +
> +static struct spi_driver efinix_spi_passive_driver = {
> +	.driver = {
> +		.name = "efnx-fpga-spi-passive",
> +		.of_match_table = of_match_ptr(efnx_spi_of_match),
> +	},
> +	.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");

Thanks,
Marco

Powered by blists - more mailing lists