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

[<Conor.Dooley@microchip.com>]

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.