| 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: <ZqUZK14ROqNvVuiO@yilunxu-OptiPlex-7050>
Date: Sat, 27 Jul 2024 23:58:35 +0800
From: Xu Yilun <yilun.xu@...ux.intel.com>
To: Ian Dannapel <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 Thu, Jul 25, 2024 at 03:44:54PM +0200, Ian Dannapel wrote:
> Hi Yilun, thanks for the review.
>
> Am Fr., 12. Juli 2024 um 09:00 Uhr schrieb Xu Yilun <yilun.xu@...ux.intel.com>:
> >
> > 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.
> sure
> >
> > > Add a new driver for loading binary firmware using "SPI passive
> >
> > Loading to some nvram or reporgraming to FPGA logic blocks.
Sorry for typo, this is a question:
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?
> to enter the passive programming mode, a reset must be executed while
> the chip select is active.
> The is controlling the chip select from here, since I expect that the
> SPI controller to only activate
> the CS when communicating.
The concern is, it may conflict with the underlying cs control in spi
controller.
There are several control flags in struct spi_transfter to affect cs. Is
there any chance using them, or try to improve if they doesn't meet your
request?
> >
> > > + usleep_range(5, 15);
> > > + reset(mgr);
> > > +
> > > + /* wait tDMIN */
> > > + usleep_range(100, 150);
And these ones, or you could use some delay controls in struct spi_transfer.
> > > +
> > > + 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?
> I assumed that the spi controller should write complete buffer and
> decide on the transfer block size,
> so there shouldn't be any remaining data. Can someone confirm?
This is not about spi transfer, it is the fpga_manager_ops.write could
be called multiple times during one time reprogramming. Please
investigate more about the FPGA mgr core.
Thanks,
Yilun
Powered by blists - more mailing lists