Re: [PATCH v2 1/3] fpga: Add Efinix Trion & Titanium serial SPI programming driver

[Xu Yilun]

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@xxxxxxxxxxxxxxx>:
> >
> > On Fri, Jun 28, 2024 at 05:23:46PM +0200, iansdannapel@xxxxxxxxx wrote:
> > > From: Ian Dannapel <iansdannapel@xxxxxxxxx>
> > >
> >
> > 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@xxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxx>
> > > + *
> > > + * 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