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 <[email protected]>
---
drivers/fpga/Kconfig | 9 +
drivers/fpga/Makefile | 1 +
drivers/fpga/microchip-spi.c | 371 +++++++++++++++++++++++++++++++++++
3 files changed, 381 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..cb59f43000a4
--- /dev/null
+++ b/drivers/fpga/microchip-spi.c
@@ -0,0 +1,371 @@
+// 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 10000
+#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 = 0, status_command = MPF_SPI_READ_STATUS;
+ struct spi_transfer xfers[] = {
+ [0 ... 1] = {
+ .tx_buf = &status_command,
+ .rx_buf = &status,
+ .len = 1,
+ .cs_change = 1,
+ }
+ };
+ int ret = spi_sync_transfer(spi, xfers, 2);
+
+ 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;
+ }
+
+ return -EBUSY;
+}
+
+static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size)
+{
+ int status = poll_status_not_busy(spi, 0);
+
+ if (status < 0)
+ return status;
+
+ return spi_write(spi, buf, buf_size);
+}
+
+static int mpf_spi_write_then_read(struct spi_device *spi,
+ const void *txbuf, size_t txbuf_size,
+ void *rxbuf, size_t rxbuf_size)
+{
+ const u8 read_command[] = { MPF_SPI_READ_DATA };
+ int ret;
+
+ ret = mpf_spi_write(spi, txbuf, txbuf_size);
+ if (ret)
+ return ret;
+
+ ret = poll_status_not_busy(spi, MPF_STATUS_READY);
+ if (ret < 0)
+ return ret;
+
+ return spi_write_then_read(spi, read_command, sizeof(read_command),
+ rxbuf, rxbuf_size);
+}
+
+static int mpf_ops_write_init(struct fpga_manager *mgr,
+ struct fpga_image_info *info, const char *buf,
+ size_t count)
+{
+ const u8 program_mode[] = { MPF_SPI_FRAME_INIT, MPF_SPI_PRG_MODE };
+ const u8 isc_en_command[] = { MPF_SPI_ISC_ENABLE };
+ struct mpf_priv *priv = mgr->priv;
+ struct device *dev = &mgr->dev;
+ struct spi_device *spi;
+ u32 isc_ret;
+ int ret;
+
+ if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
+ dev_err(dev, "Partial reconfiguration is not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ spi = priv->spi;
+
+ ret = mpf_spi_write_then_read(spi, isc_en_command, sizeof(isc_en_command),
+ &isc_ret, sizeof(isc_ret));
+ if (ret || isc_ret) {
+ dev_err(dev, "Failed to enable ISC: %d\n", ret ? : isc_ret);
+ return -EFAULT;
+ }
+
+ ret = mpf_spi_write(spi, program_mode, sizeof(program_mode));
+ if (ret) {
+ dev_err(dev, "Failed to enter program mode: %d\n", ret);
+ return ret;
+ }
+
+ priv->program_mode = true;
+
+ return 0;
+}
+
+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/%zd\n",
+ i, count / MPF_SPI_FRAME_SIZE);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int mpf_ops_write_complete(struct fpga_manager *mgr,
+ struct fpga_image_info *info)
+{
+ const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
+ const u8 release_command[] = { MPF_SPI_RELEASE };
+ struct mpf_priv *priv = mgr->priv;
+ struct device *dev = &mgr->dev;
+ struct spi_device *spi;
+ int ret;
+
+ spi = priv->spi;
+
+ ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
+ if (ret) {
+ dev_err(dev, "Failed to disable ISC: %d\n", ret);
+ return ret;
+ }
+
+ usleep_range(1000, 2000);
+
+ ret = mpf_spi_write(spi, release_command, sizeof(release_command));
+ if (ret) {
+ dev_err(dev, "Failed to exit program mode: %d\n", ret);
+ return ret;
+ }
+
+ priv->program_mode = false;
+
+ return 0;
+}
+
+static const struct fpga_manager_ops mpf_ops = {
+ .state = mpf_ops_state,
+ .initial_header_size = 71,
+ .parse_header = mpf_ops_parse_header,
+ .write_init = mpf_ops_write_init,
+ .write = mpf_ops_write,
+ .write_complete = mpf_ops_write_complete,
+};
+
+static int mpf_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct fpga_manager *mgr;
+ struct mpf_priv *priv;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->spi = spi;
+
+ mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
+ &mpf_ops, priv);
+
+ return PTR_ERR_OR_ZERO(mgr);
+}
+
+static const struct spi_device_id mpf_spi_ids[] = {
+ { .name = "mpf-spi-fpga-mgr", },
+ {},
+};
+MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id mpf_of_ids[] = {
+ { .compatible = "microchip,mpf-spi-fpga-mgr" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mpf_of_ids);
+#endif /* IS_ENABLED(CONFIG_OF) */
+
+static struct spi_driver mpf_driver = {
+ .probe = mpf_probe,
+ .id_table = mpf_spi_ids,
+ .driver = {
+ .name = "microchip_mpf_spi_fpga_mgr",
+ .of_match_table = of_match_ptr(mpf_of_ids),
+ },
+};
+
+module_spi_driver(mpf_driver);
+
+MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
+MODULE_LICENSE("GPL");
--
2.35.1
On Mon, May 23, 2022 at 12:52:32AM +0800, Xu Yilun wrote:
> On Fri, May 13, 2022 at 07:27:54PM +0300, Ivan Bornyakov wrote:
> > 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 <[email protected]>
> > ---
> > drivers/fpga/Kconfig | 9 +
> > drivers/fpga/Makefile | 1 +
> > drivers/fpga/microchip-spi.c | 371 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 381 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..cb59f43000a4
> > --- /dev/null
> > +++ b/drivers/fpga/microchip-spi.c
> > @@ -0,0 +1,371 @@
> > +// 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 10000
> > +#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 = 0, status_command = MPF_SPI_READ_STATUS;
> > + struct spi_transfer xfers[] = {
> > + [0 ... 1] = {
> > + .tx_buf = &status_command,
> > + .rx_buf = &status,
> > + .len = 1,
> > + .cs_change = 1,
> > + }
> > + };
> > + int ret = spi_sync_transfer(spi, xfers, 2);
>
> I remember it is spi_w8r8 for the first time, why we change to
> spi_sync_transfer? They behavior differently on spi bus.
On v8 Conor reported that spi_w8r8 was not correct way to read the status,
despite that my HW was giving reasonable result.
See https://lore.kernel.org/linux-fpga/[email protected]/#t
>
> And why we need to xfer the same message twice? If it relates to
> some HW behavior, we'd better add some comments here.
>
On v11 Conor reported that he observed inadequate status readings, and
double status reads fixed that. There is also a hint in Microchip's
"SPI-DirectC User Guide" that status should be read two times, but not a
clear statement.
See https://lore.kernel.org/linux-fpga/[email protected]/#t
Anyway, I'll add some words.
> > +
> > + 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;
>
> The initialization should be listed after declaration, so swap the 2
> lines
>
> Why the component_size_byte_num, component_size_byte_off, i should be size_t?
It's a safe bet for offsets inside image buffer.
> > + 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;
>
> Why doesn't check the components_size_start against count, any difference
> with bitstream_start?
bitstream_start is in fact a synonym for info->header_size, while
components_size_start is somewhere in the header. Thus, data at offset
'components_size_start' is accessed when count is not less then
info->header_size.
> > + 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);
>
> Some check against count?
Same, also see comment just above for-loop.
> > + 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)
>
> The function name is confusing, it does more than polling busy bit.
> mpf_poll_status is better?
Well, my intention was to denote that we poll status until busy bit is
not set. I have no strong feelings for this function naming, tho.
> > +{
> > + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> > +
> > + while (timeout--) {
> > + status = mpf_read_status(spi);
> > + if (status < 0 ||
> > + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
>
> Please make the judgements easier to read.
>
> if (status < 0)
> return status;
>
> if (status & MPF_STATUS_BUSY)
> continue;
>
> if (!mask || (status & mask))
> return status;
>
> Is that OK for you?
Yeah, sure.
> > + return status;
> > + }
> > +
> > + return -EBUSY;
> > +}
> > +
> > +static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size)
> > +{
> > + int status = poll_status_not_busy(spi, 0);
> > +
> > + if (status < 0)
> > + return status;
> > +
> > + return spi_write(spi, buf, buf_size);
> > +}
> > +
> > +static int mpf_spi_write_then_read(struct spi_device *spi,
> > + const void *txbuf, size_t txbuf_size,
> > + void *rxbuf, size_t rxbuf_size)
> > +{
> > + const u8 read_command[] = { MPF_SPI_READ_DATA };
> > + int ret;
> > +
> > + ret = mpf_spi_write(spi, txbuf, txbuf_size);
> > + if (ret)
> > + return ret;
> > +
> > + ret = poll_status_not_busy(spi, MPF_STATUS_READY);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return spi_write_then_read(spi, read_command, sizeof(read_command),
> > + rxbuf, rxbuf_size);
> > +}
> > +
> > +static int mpf_ops_write_init(struct fpga_manager *mgr,
> > + struct fpga_image_info *info, const char *buf,
> > + size_t count)
> > +{
> > + const u8 program_mode[] = { MPF_SPI_FRAME_INIT, MPF_SPI_PRG_MODE };
> > + const u8 isc_en_command[] = { MPF_SPI_ISC_ENABLE };
> > + struct mpf_priv *priv = mgr->priv;
> > + struct device *dev = &mgr->dev;
> > + struct spi_device *spi;
> > + u32 isc_ret;
> > + int ret;
> > +
> > + if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> > + dev_err(dev, "Partial reconfiguration is not supported\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + spi = priv->spi;
> > +
> > + ret = mpf_spi_write_then_read(spi, isc_en_command, sizeof(isc_en_command),
> > + &isc_ret, sizeof(isc_ret));
> > + if (ret || isc_ret) {
> > + dev_err(dev, "Failed to enable ISC: %d\n", ret ? : isc_ret);
>
> %d for a u32 variable? Maybe just list the 2 values.
Ok, will do.
> > + return -EFAULT;
> > + }
> > +
> > + ret = mpf_spi_write(spi, program_mode, sizeof(program_mode));
> > + if (ret) {
> > + dev_err(dev, "Failed to enter program mode: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + priv->program_mode = true;
> > +
> > + return 0;
> > +}
> > +
> > +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));
>
> As I mentioned before, is it possible we use spi_sync_transfer to avoid
> memcpy the whole bitstream?
Unfortunately, I didn't succeed with spi_sunc_transfer here. May be
Conor or other folks with more insight on Microchip's HW would be able
to eliminate this memcpy...
> > + if (ret) {
> > + dev_err(dev, "Failed to write bitstream frame %d/%zd\n",
>
> %zu for size_t
>
> > + i, count / MPF_SPI_FRAME_SIZE);
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mpf_ops_write_complete(struct fpga_manager *mgr,
> > + struct fpga_image_info *info)
> > +{
> > + const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
> > + const u8 release_command[] = { MPF_SPI_RELEASE };
> > + struct mpf_priv *priv = mgr->priv;
> > + struct device *dev = &mgr->dev;
> > + struct spi_device *spi;
> > + int ret;
> > +
> > + spi = priv->spi;
> > +
> > + ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
> > + if (ret) {
> > + dev_err(dev, "Failed to disable ISC: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + usleep_range(1000, 2000);
> > +
> > + ret = mpf_spi_write(spi, release_command, sizeof(release_command));
> > + if (ret) {
> > + dev_err(dev, "Failed to exit program mode: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + priv->program_mode = false;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct fpga_manager_ops mpf_ops = {
> > + .state = mpf_ops_state,
> > + .initial_header_size = 71,
> > + .parse_header = mpf_ops_parse_header,
> > + .write_init = mpf_ops_write_init,
> > + .write = mpf_ops_write,
> > + .write_complete = mpf_ops_write_complete,
> > +};
> > +
> > +static int mpf_probe(struct spi_device *spi)
> > +{
> > + struct device *dev = &spi->dev;
> > + struct fpga_manager *mgr;
> > + struct mpf_priv *priv;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->spi = spi;
> > +
> > + mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
> > + &mpf_ops, priv);
> > +
> > + return PTR_ERR_OR_ZERO(mgr);
> > +}
> > +
> > +static const struct spi_device_id mpf_spi_ids[] = {
> > + { .name = "mpf-spi-fpga-mgr", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
> > +
> > +#if IS_ENABLED(CONFIG_OF)
> > +static const struct of_device_id mpf_of_ids[] = {
> > + { .compatible = "microchip,mpf-spi-fpga-mgr" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, mpf_of_ids);
> > +#endif /* IS_ENABLED(CONFIG_OF) */
> > +
> > +static struct spi_driver mpf_driver = {
> > + .probe = mpf_probe,
> > + .id_table = mpf_spi_ids,
> > + .driver = {
> > + .name = "microchip_mpf_spi_fpga_mgr",
> > + .of_match_table = of_match_ptr(mpf_of_ids),
> > + },
> > +};
> > +
> > +module_spi_driver(mpf_driver);
> > +
> > +MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.35.1
> >
On Fri, May 13, 2022 at 07:27:54PM +0300, Ivan Bornyakov wrote:
> 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 <[email protected]>
> ---
> drivers/fpga/Kconfig | 9 +
> drivers/fpga/Makefile | 1 +
> drivers/fpga/microchip-spi.c | 371 +++++++++++++++++++++++++++++++++++
> 3 files changed, 381 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..cb59f43000a4
> --- /dev/null
> +++ b/drivers/fpga/microchip-spi.c
> @@ -0,0 +1,371 @@
> +// 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 10000
> +#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 = 0, status_command = MPF_SPI_READ_STATUS;
> + struct spi_transfer xfers[] = {
> + [0 ... 1] = {
> + .tx_buf = &status_command,
> + .rx_buf = &status,
> + .len = 1,
> + .cs_change = 1,
> + }
> + };
> + int ret = spi_sync_transfer(spi, xfers, 2);
I remember it is spi_w8r8 for the first time, why we change to
spi_sync_transfer? They behavior differently on spi bus.
And why we need to xfer the same message twice? If it relates to
some HW behavior, we'd better add some comments here.
> +
> + 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;
The initialization should be listed after declaration, so swap the 2
lines
Why the component_size_byte_num, component_size_byte_off, i should be size_t?
> + 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;
Why doesn't check the components_size_start against count, any difference
with bitstream_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);
Some check against count?
> + 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)
The function name is confusing, it does more than polling busy bit.
mpf_poll_status is better?
> +{
> + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> +
> + while (timeout--) {
> + status = mpf_read_status(spi);
> + if (status < 0 ||
> + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
Please make the judgements easier to read.
if (status < 0)
return status;
if (status & MPF_STATUS_BUSY)
continue;
if (!mask || (status & mask))
return status;
Is that OK for you?
> + return status;
> + }
> +
> + return -EBUSY;
> +}
> +
> +static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size)
> +{
> + int status = poll_status_not_busy(spi, 0);
> +
> + if (status < 0)
> + return status;
> +
> + return spi_write(spi, buf, buf_size);
> +}
> +
> +static int mpf_spi_write_then_read(struct spi_device *spi,
> + const void *txbuf, size_t txbuf_size,
> + void *rxbuf, size_t rxbuf_size)
> +{
> + const u8 read_command[] = { MPF_SPI_READ_DATA };
> + int ret;
> +
> + ret = mpf_spi_write(spi, txbuf, txbuf_size);
> + if (ret)
> + return ret;
> +
> + ret = poll_status_not_busy(spi, MPF_STATUS_READY);
> + if (ret < 0)
> + return ret;
> +
> + return spi_write_then_read(spi, read_command, sizeof(read_command),
> + rxbuf, rxbuf_size);
> +}
> +
> +static int mpf_ops_write_init(struct fpga_manager *mgr,
> + struct fpga_image_info *info, const char *buf,
> + size_t count)
> +{
> + const u8 program_mode[] = { MPF_SPI_FRAME_INIT, MPF_SPI_PRG_MODE };
> + const u8 isc_en_command[] = { MPF_SPI_ISC_ENABLE };
> + struct mpf_priv *priv = mgr->priv;
> + struct device *dev = &mgr->dev;
> + struct spi_device *spi;
> + u32 isc_ret;
> + int ret;
> +
> + if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> + dev_err(dev, "Partial reconfiguration is not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + spi = priv->spi;
> +
> + ret = mpf_spi_write_then_read(spi, isc_en_command, sizeof(isc_en_command),
> + &isc_ret, sizeof(isc_ret));
> + if (ret || isc_ret) {
> + dev_err(dev, "Failed to enable ISC: %d\n", ret ? : isc_ret);
%d for a u32 variable? Maybe just list the 2 values.
> + return -EFAULT;
> + }
> +
> + ret = mpf_spi_write(spi, program_mode, sizeof(program_mode));
> + if (ret) {
> + dev_err(dev, "Failed to enter program mode: %d\n", ret);
> + return ret;
> + }
> +
> + priv->program_mode = true;
> +
> + return 0;
> +}
> +
> +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));
As I mentioned before, is it possible we use spi_sync_transfer to avoid
memcpy the whole bitstream?
> + if (ret) {
> + dev_err(dev, "Failed to write bitstream frame %d/%zd\n",
%zu for size_t
> + i, count / MPF_SPI_FRAME_SIZE);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int mpf_ops_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
> + const u8 release_command[] = { MPF_SPI_RELEASE };
> + struct mpf_priv *priv = mgr->priv;
> + struct device *dev = &mgr->dev;
> + struct spi_device *spi;
> + int ret;
> +
> + spi = priv->spi;
> +
> + ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
> + if (ret) {
> + dev_err(dev, "Failed to disable ISC: %d\n", ret);
> + return ret;
> + }
> +
> + usleep_range(1000, 2000);
> +
> + ret = mpf_spi_write(spi, release_command, sizeof(release_command));
> + if (ret) {
> + dev_err(dev, "Failed to exit program mode: %d\n", ret);
> + return ret;
> + }
> +
> + priv->program_mode = false;
> +
> + return 0;
> +}
> +
> +static const struct fpga_manager_ops mpf_ops = {
> + .state = mpf_ops_state,
> + .initial_header_size = 71,
> + .parse_header = mpf_ops_parse_header,
> + .write_init = mpf_ops_write_init,
> + .write = mpf_ops_write,
> + .write_complete = mpf_ops_write_complete,
> +};
> +
> +static int mpf_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct fpga_manager *mgr;
> + struct mpf_priv *priv;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->spi = spi;
> +
> + mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
> + &mpf_ops, priv);
> +
> + return PTR_ERR_OR_ZERO(mgr);
> +}
> +
> +static const struct spi_device_id mpf_spi_ids[] = {
> + { .name = "mpf-spi-fpga-mgr", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id mpf_of_ids[] = {
> + { .compatible = "microchip,mpf-spi-fpga-mgr" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, mpf_of_ids);
> +#endif /* IS_ENABLED(CONFIG_OF) */
> +
> +static struct spi_driver mpf_driver = {
> + .probe = mpf_probe,
> + .id_table = mpf_spi_ids,
> + .driver = {
> + .name = "microchip_mpf_spi_fpga_mgr",
> + .of_match_table = of_match_ptr(mpf_of_ids),
> + },
> +};
> +
> +module_spi_driver(mpf_driver);
> +
> +MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
> +MODULE_LICENSE("GPL");
> --
> 2.35.1
>
On 22/05/2022 21:22, Ivan Bornyakov wrote:
> On Mon, May 23, 2022 at 12:52:32AM +0800, Xu Yilun wrote:
>> On Fri, May 13, 2022 at 07:27:54PM +0300, Ivan Bornyakov wrote:
>>> Add support to the FPGA manager for programming Microchip Polarfire
>>> FPGAs over slave SPI interface with .dat formatted bitsream image.
---8<---
>>> +static int mpf_read_status(struct spi_device *spi)
>>> +{
>>> + u8 status = 0, status_command = MPF_SPI_READ_STATUS;
>>> + struct spi_transfer xfers[] = {
>>> + [0 ... 1] = {
>>> + .tx_buf = &status_command,
>>> + .rx_buf = &status,
>>> + .len = 1,
>>> + .cs_change = 1,
>>> + }
>>> + };
>>> + int ret = spi_sync_transfer(spi, xfers, 2);
>>
>> I remember it is spi_w8r8 for the first time, why we change to
>> spi_sync_transfer? They behavior differently on spi bus.
>
> On v8 Conor reported that spi_w8r8 was not correct way to read the status,
> despite that my HW was giving reasonable result.
>
> See https://lore.kernel.org/linux-fpga/[email protected]/#t
For some context, the spi slave programming was implemented to match the
jtag programming process. The status can't be read with w8r8() because it
comes directly from the hardware between the pins on the FPGA and the spi
slave & will start clocking it out onto MISO as soon as it has a clock
and its select is active.
I am not really sure how it worked that way for you.
>
>>
>> And why we need to xfer the same message twice? If it relates to
>> some HW behavior, we'd better add some comments here.
>>
>
> On v11 Conor reported that he observed inadequate status readings, and
> double status reads fixed that. There is also a hint in Microchip's
> "SPI-DirectC User Guide" that status should be read two times, but not a
> clear statement.
>
> See https://lore.kernel.org/linux-fpga/[email protected]/#t
>
> Anyway, I'll add some words.
I'll be honest and say that the DirectC user guide is not great, I had
a look for that "hint" but it didn't stand out to me.
Unless that is you meant two screenshots of "checking hardware status"
at this URL in section 10's subsections?
https://onlinedocs.microchip.com/pr/GUID-835917AF-E521-4046-AD59-DCB458EB8466-en-US-1/index.html
If that is the case, that seems very insufficient to me. I will try to
get clarification & suggest that the documentation be improved if so.
---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));
>>
>> As I mentioned before, is it possible we use spi_sync_transfer to avoid
>> memcpy the whole bitstream?
>
> Unfortunately, I didn't succeed with spi_sunc_transfer here. May be
> Conor or other folks with more insight on Microchip's HW would be able
> to eliminate this memcpy...
Sure, I'll take a look. If I can't answer I'll try to find out.
I've been really busy the last week (and will be this coming week too) so
while I did use v12, I have not been able to properly sit down and look
at it properly yet, so apologies for the radio silence from me. Hopefully
I will get a chance this week & can give either a review or ack etc.
Thanks,
Conor.
>
>>> + if (ret) {
>>> + dev_err(dev, "Failed to write bitstream frame %d/%zd\n",
>>
>> %zu for size_t
>>
>>> + i, count / MPF_SPI_FRAME_SIZE);
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int mpf_ops_write_complete(struct fpga_manager *mgr,
>>> + struct fpga_image_info *info)
>>> +{
>>> + const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
>>> + const u8 release_command[] = { MPF_SPI_RELEASE };
>>> + struct mpf_priv *priv = mgr->priv;
>>> + struct device *dev = &mgr->dev;
>>> + struct spi_device *spi;
>>> + int ret;
>>> +
>>> + spi = priv->spi;
>>> +
>>> + ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
>>> + if (ret) {
>>> + dev_err(dev, "Failed to disable ISC: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + usleep_range(1000, 2000);
>>> +
>>> + ret = mpf_spi_write(spi, release_command, sizeof(release_command));
>>> + if (ret) {
>>> + dev_err(dev, "Failed to exit program mode: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + priv->program_mode = false;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct fpga_manager_ops mpf_ops = {
>>> + .state = mpf_ops_state,
>>> + .initial_header_size = 71,
>>> + .parse_header = mpf_ops_parse_header,
>>> + .write_init = mpf_ops_write_init,
>>> + .write = mpf_ops_write,
>>> + .write_complete = mpf_ops_write_complete,
>>> +};
>>> +
>>> +static int mpf_probe(struct spi_device *spi)
>>> +{
>>> + struct device *dev = &spi->dev;
>>> + struct fpga_manager *mgr;
>>> + struct mpf_priv *priv;
>>> +
>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>> +
>>> + priv->spi = spi;
>>> +
>>> + mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
>>> + &mpf_ops, priv);
>>> +
>>> + return PTR_ERR_OR_ZERO(mgr);
>>> +}
>>> +
>>> +static const struct spi_device_id mpf_spi_ids[] = {
>>> + { .name = "mpf-spi-fpga-mgr", },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
>>> +
>>> +#if IS_ENABLED(CONFIG_OF)
>>> +static const struct of_device_id mpf_of_ids[] = {
>>> + { .compatible = "microchip,mpf-spi-fpga-mgr" },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, mpf_of_ids);
>>> +#endif /* IS_ENABLED(CONFIG_OF) */
>>> +
>>> +static struct spi_driver mpf_driver = {
>>> + .probe = mpf_probe,
>>> + .id_table = mpf_spi_ids,
>>> + .driver = {
>>> + .name = "microchip_mpf_spi_fpga_mgr",
>>> + .of_match_table = of_match_ptr(mpf_of_ids),
>>> + },
>>> +};
>>> +
>>> +module_spi_driver(mpf_driver);
>>> +
>>> +MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.35.1
>>>
>