Re: [PATCH v14 2/3] fpga: microchip-spi: add Microchip MPF FPGA manager

[Ivan Bornyakov <i.bornyakov@metrotek.ru>]

On Wed, Jun 01, 2022 at 07:49:48AM +0000, Conor.Dooley@microchip.com wrote:
> On 31/05/2022 20:23, Ivan Bornyakov wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Add support to the FPGA manager for programming Microchip Polarfire
> > FPGAs over slave SPI interface with .dat formatted bitsream image.
> > 
> > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> 
> Hey Ivan,
> Could've kept at least the R-b tag from v13, but either way:
> Tested-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> I did have another look at Yiluns two questions.
> The reason for the double status read effectively boils down
> to the system controller potentially not having had enough
> time to process the previous command/frame and update its
> status yet.
> 
> And see below about the memcpy()...
> 
> > ---
> >   drivers/fpga/Kconfig         |   9 +
> >   drivers/fpga/Makefile        |   1 +
> >   drivers/fpga/microchip-spi.c | 384 +++++++++++++++++++++++++++++++++++
> >   3 files changed, 394 insertions(+)
> >   create mode 100644 drivers/fpga/microchip-spi.c
> > 
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 26025dbab353..75806ef5c9ea 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -248,4 +248,13 @@ config FPGA_MGR_VERSAL_FPGA
> >            configure the programmable logic(PL).
> > 
> >            To compile this as a module, choose M here.
> > +
> > +config FPGA_MGR_MICROCHIP_SPI
> > +       tristate "Microchip Polarfire SPI FPGA manager"
> > +       depends on SPI
> > +       help
> > +         FPGA manager driver support for Microchip Polarfire FPGAs
> > +         programming over slave SPI interface with .dat formatted
> > +         bitstream image.
> > +
> >   endif # FPGA
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index e32bfa90f968..5425a15892df 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI)     += xilinx-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
> > +obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI)   += microchip-spi.o
> >   obj-$(CONFIG_ALTERA_PR_IP_CORE)                += altera-pr-ip-core.o
> >   obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)   += altera-pr-ip-core-plat.o
> > 
> > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> > new file mode 100644
> > index 000000000000..413e5c364f68
> ---8<---
> > +
> > +static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
> > +{
> > +       u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
> > +       struct mpf_priv *priv = mgr->priv;
> > +       struct device *dev = &mgr->dev;
> > +       struct spi_device *spi;
> > +       int ret, i;
> > +
> > +       if (count % MPF_SPI_FRAME_SIZE) {
> > +               dev_err(dev, "Bitstream size is not a multiple of %d\n",
> > +                       MPF_SPI_FRAME_SIZE);
> > +               return -EINVAL;
> > +       }
> > +
> > +       spi = priv->spi;
> > +
> > +       for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
> > +               memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
> > +                      MPF_SPI_FRAME_SIZE);
> > +
> > +               ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
> > +               if (ret) {
> > +                       dev_err(dev, "Failed to write bitstream frame %d/%zu\n",
> > +                               i, count / MPF_SPI_FRAME_SIZE);
> > +                       return ret;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> 
> I was able to program with the following diff, which is effectively
> the same as the one you sent (which also worked for me):
> 
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> index 413e5c364f68..fa6220e8056a 100644
> --- a/drivers/fpga/microchip-spi.c
> +++ b/drivers/fpga/microchip-spi.c
> @@ -268,7 +268,8 @@ static int mpf_ops_write_init(struct fpga_manager *mgr,
>   
>   static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
>   {
> -       u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
> +       u8 spi_frame_command = MPF_SPI_FRAME;
> +       struct spi_transfer xfers[2] = { 0 };
>          struct mpf_priv *priv = mgr->priv;
>          struct device *dev = &mgr->dev;
>          struct spi_device *spi;
> @@ -282,11 +283,16 @@ static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count
>   
>          spi = priv->spi;
>   
> +       xfers[0].tx_buf = &spi_frame_command;
> +       xfers[0].len = 1;
> +
>          for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
> -               memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
> -                      MPF_SPI_FRAME_SIZE);
> +               xfers[1].tx_buf = buf + i * MPF_SPI_FRAME_SIZE;
> +               xfers[1].len = MPF_SPI_FRAME_SIZE;
>   
> -               ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
> +               ret = mpf_poll_status(spi, 0);
> +               if (ret >= 0)
> +                       ret = spi_sync_transfer(spi, xfers, 2);
>                  if (ret) {
>                          dev_err(dev, "Failed to write bitstream frame %d/%zu\n",
>                                  i, count / MPF_SPI_FRAME_SIZE);
> 
> Have you been able to look at a waveform of programming like this
> on your hardware? Off the top of my head, I was wondering if your
> spi controller releases CS between every xfer?
> Mine is perfectly happy not to release it - which is why I noticed
> the extra cs_change in mpf_read_status().
> 
> Thanks,
> Conor.
> 

Thanks for the hint. I'll ask our HW folks if it's possible.