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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hey Ivan, one comment below.
Thanks,
Conor.

On 07/05/2022 08:43, 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@xxxxxxxxxxx>
> ---
>   drivers/fpga/Kconfig         |   9 +
>   drivers/fpga/Makefile        |   1 +
>   drivers/fpga/microchip-spi.c | 370 +++++++++++++++++++++++++++++++++++
>   3 files changed, 380 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..182471c88018
> --- /dev/null
> +++ b/drivers/fpga/microchip-spi.c
> @@ -0,0 +1,370 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip Polarfire FPGA programming over slave SPI interface.
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +
> +#define        MPF_SPI_ISC_ENABLE      0x0B
> +#define        MPF_SPI_ISC_DISABLE     0x0C
> +#define        MPF_SPI_READ_STATUS     0x00
> +#define        MPF_SPI_READ_DATA       0x01
> +#define        MPF_SPI_FRAME_INIT      0xAE
> +#define        MPF_SPI_FRAME           0xEE
> +#define        MPF_SPI_PRG_MODE        0x01
> +#define        MPF_SPI_RELEASE         0x23
> +
> +#define        MPF_SPI_FRAME_SIZE      16
> +
> +#define        MPF_HEADER_SIZE_OFFSET  24
> +#define        MPF_DATA_SIZE_OFFSET    55
> +
> +#define        MPF_LOOKUP_TABLE_RECORD_SIZE            9
> +#define        MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET        0
> +#define        MPF_LOOKUP_TABLE_BLOCK_START_OFFSET     1
> +
> +#define        MPF_COMPONENTS_SIZE_ID  5
> +#define        MPF_BITSTREAM_ID        8
> +
> +#define        MPF_BITS_PER_COMPONENT_SIZE     22
> +
> +#define        MPF_STATUS_POLL_TIMEOUT         1000
> +#define        MPF_STATUS_BUSY                 BIT(0)
> +#define        MPF_STATUS_READY                BIT(1)
> +#define        MPF_STATUS_SPI_VIOLATION        BIT(2)
> +#define        MPF_STATUS_SPI_ERROR            BIT(3)
> +
> +struct mpf_priv {
> +       struct spi_device *spi;
> +       bool program_mode;
> +};
> +
> +static int mpf_read_status(struct spi_device *spi)
> +{
> +       u8 status, status_command = MPF_SPI_READ_STATUS;
> +       struct spi_transfer xfer = {
> +               .tx_buf = &status_command,
> +               .rx_buf = &status,
> +               .len = 1,
> +       };
> +       int ret = spi_sync_transfer(spi, &xfer, 1);
> +
> +       if ((status & MPF_STATUS_SPI_VIOLATION) ||
> +           (status & MPF_STATUS_SPI_ERROR))
> +               ret = -EIO;
> +
> +       return ret ? : status;
> +}
> +
> +static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr)
> +{
> +       struct mpf_priv *priv = mgr->priv;
> +       struct spi_device *spi;
> +       bool program_mode;
> +       int status;
> +
> +       spi = priv->spi;
> +       program_mode = priv->program_mode;
> +       status = mpf_read_status(spi);
> +
> +       if (!program_mode && !status)
> +               return FPGA_MGR_STATE_OPERATING;
> +
> +       return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int mpf_ops_parse_header(struct fpga_manager *mgr,
> +                               struct fpga_image_info *info,
> +                               const char *buf, size_t count)
> +{
> +       size_t component_size_byte_num, component_size_byte_off,
> +              components_size_start = 0, bitstream_start = 0,
> +              block_id_offset, block_start_offset, i;
> +       u8 header_size, blocks_num, block_id;
> +       u32 block_start, component_size;
> +       u16 components_num;
> +
> +       if (!buf) {
> +               dev_err(&mgr->dev, "Image buffer is not provided\n");
> +               return -EINVAL;
> +       }
> +
> +       header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
> +       if (header_size > count) {
> +               info->header_size = header_size;
> +               return -EAGAIN;
> +       }
> +
> +       /*
> +        * Go through look-up table to find out where actual bitstream starts
> +        * and where sizes of components of the bitstream lies.
> +        */
> +       blocks_num = *(buf + header_size - 1);
> +       block_id_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
> +       block_start_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
> +
> +       header_size += blocks_num * MPF_LOOKUP_TABLE_RECORD_SIZE;
> +       if (header_size > count) {
> +               info->header_size = header_size;
> +               return -EAGAIN;
> +       }
> +
> +       while (blocks_num--) {
> +               block_id = *(buf + block_id_offset);
> +               block_start = get_unaligned_le32(buf + block_start_offset);
> +
> +               switch (block_id) {
> +               case MPF_BITSTREAM_ID:
> +                       info->header_size = bitstream_start = block_start;
> +                       if (block_start > count)
> +                               return -EAGAIN;
> +
> +                       break;
> +               case MPF_COMPONENTS_SIZE_ID:
> +                       components_size_start = block_start;
> +                       break;
> +               default:
> +                       break;
> +               }
> +
> +               if (bitstream_start && components_size_start)
> +                       break;
> +
> +               block_id_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
> +               block_start_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
> +       }
> +
> +       if (!bitstream_start || !components_size_start) {
> +               dev_err(&mgr->dev, "Failed to parse header look-up table\n");
> +               return -EFAULT;
> +       }
> +
> +       /*
> +        * Parse bitstream size.
> +        * Sizes of components of the bitstream are 22-bits long placed next
> +        * to each other. Image header should be extended by now up to where
> +        * actual bitstream starts, so no need for overflow check anymore.
> +        */
> +       components_num = get_unaligned_le16(buf + MPF_DATA_SIZE_OFFSET);
> +
> +       for (i = 0; i < components_num; i++) {
> +               component_size_byte_num =
> +                       (i * MPF_BITS_PER_COMPONENT_SIZE) / BITS_PER_BYTE;
> +               component_size_byte_off =
> +                       (i * MPF_BITS_PER_COMPONENT_SIZE) % BITS_PER_BYTE;
> +
> +               component_size = get_unaligned_le32(buf +
> +                                                   components_size_start +
> +                                                   component_size_byte_num);
> +               component_size >>= component_size_byte_off;
> +               component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
> +
> +               info->data_size += component_size * MPF_SPI_FRAME_SIZE;
> +       }
> +
> +       return 0;
> +}
> +
> +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> +{
> +       int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> +
> +       while (timeout--) {
> +               status = mpf_read_status(spi);
> +               if (status < 0 ||
> +                   (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> +                       return status;
> +
> +               usleep_range(1000, 2000);
> +       }
> +
> +       return -EBUSY;
> +}

Is there a reason you changed this from the snippet you sent me
in the responses to version 8:
static int poll_status_not_busy(struct spi_device *spi, u8 mask)
{
	u8 status, status_command = MPF_SPI_READ_STATUS;
	int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
	struct spi_transfer xfer = {
		.tx_buf = &status_command,
		.rx_buf = &status,
		.len = 1,
	};

	while (timeout--) {
		ret = spi_sync_transfer(spi, &xfer, 1);
		if (ret < 0)
			return ret;

		if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
			return status;

		usleep_range(1000, 2000);
	}

	return -EBUSY;
}

With the current version, I hit the "Failed to write bitstream
frame" check in mpf_ops_write at random points in the transfer.
Replacing poll_status_not_busy with the above allows it to run
to completion.




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux