| 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: <CAKrir7h4bSMQ=TkP=a+4sG6OK7NVn=dj2k6ypCF6TJUd_JiOcw@mail.gmail.com>
Date: Fri, 9 Aug 2024 12:38:03 +0200
From: Ian Dannapel <iansdannapel@...il.com>
To: Xu Yilun <yilun.xu@...ux.intel.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
Am Mo., 29. Juli 2024 um 16:52 Uhr schrieb Xu Yilun <yilun.xu@...ux.intel.com>:
>
> On Sun, Jul 28, 2024 at 01:44:03PM +0200, Ian Dannapel wrote:
> > Am Sa., 27. Juli 2024 um 18:00 Uhr schrieb Xu Yilun <yilun.xu@...ux.intel.com>:
> > >
> > > 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?
> > The datasheet refers to it as configuration RAM (CRAM) and must be
> > loaded on every boot up.
>
> The FPGA reprogramming is about loading the FPGA logic blocks, and from
> the POV of the System, it is about runtime changing the HW and
> re-enumerate the devices.
>
> But some submitted fpga-mgr drivers are just used to to write nvram
> backend for FPGA, don't affect any running device at all. I think these
> drivers virtually have nothing to do with fpga-mgr. Some generic
> storage (e.g. nvram, mtd) or firmware image loading interfaces better
> fit them. I assume this driver is also of this kind.
To clarify, this driver does not write to a nvram or any persistent
memory. Programming or reprogramming at runtime is necessary, but
partial reprogramming is not supported.
>
> > >
> > > > >
> > > > > > 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?
> > The main problem of this chip is that probably the of SPI is out of
> > spec, so would like to avoid changes in the spi contoller for this
> > edge case.
> > That is probably one the reasons that the datasheet does not recommend
> > other devices on the same SPI bus.
>
> I think anyway you need to communicate with spi controller driver and
> have one voice how/who to take control of cs, rather than blindly
> operate at both sides.
I did not find a clear solution yet. This FGPA device requires an SPI
without a chip-select signal and no other device transaction should be
allowed in between reprogramming, which seems to be a very specific
case for the SPI Controller to manage
>
> Thanks,
> Yilun
Thank
Ian
Powered by blists - more mailing lists