* [PATCH v8 0/2] Microchip Polarfire FPGA manager
@ 2022-03-22 19:15 Ivan Bornyakov
2022-03-22 19:15 ` [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF " Ivan Bornyakov
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ivan Bornyakov @ 2022-03-22 19:15 UTC (permalink / raw)
Cc: Ivan Bornyakov, mdf, hao.wu, yilun.xu, trix, conor.dooley,
robh+dt, system, linux-kernel, linux-fpga, devicetree
Add support to the FPGA manager for programming Microchip Polarfire
FPGAs over slave SPI interface with .dat formatted bitsream image.
Changelog:
v1 -> v2: fix printk formating
v2 -> v3:
* replace "microsemi" with "microchip"
* replace prefix "microsemi_fpga_" with "mpf_"
* more sensible .compatible and .name strings
* remove unused defines STATUS_SPI_VIOLATION and STATUS_SPI_ERROR
v3 -> v4: fix unused variable warning
Put 'mpf_of_ids' definition under conditional compilation, so it
would not hang unused if CONFIG_OF is not enabled.
v4 -> v5:
* prefix defines with MPF_
* mdelay() -> usleep_range()
* formatting fixes
* add DT bindings doc
* rework fpga_manager_ops.write() to fpga_manager_ops.write_sg()
We can't parse image header in write_init() because image header
size is not known beforehand. Thus parsing need to be done in
fpga_manager_ops.write() callback, but fpga_manager_ops.write()
also need to be reenterable. On the other hand,
fpga_manager_ops.write_sg() is called once. Thus, rework usage of
write() callback to write_sg().
v5 -> v6: fix patch applying
I forgot to clean up unrelated local changes which lead to error on
patch 0001-fpga-microchip-spi-add-Microchip-MPF-FPGA-manager.patch
applying on vanilla kernel.
v6 -> v7: fix binding doc to pass dt_binding_check
v7 -> v8: another fix for dt_binding_check warning
Ivan Bornyakov (2):
fpga: microchip-spi: add Microchip MPF FPGA manager
dt-bindings: fpga: add binding doc for microchip-spi fpga mgr
.../fpga/microchip,mpf-spi-fpga-mgr.yaml | 44 ++
drivers/fpga/Kconfig | 10 +
drivers/fpga/Makefile | 1 +
drivers/fpga/microchip-spi.c | 448 ++++++++++++++++++
4 files changed, 503 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
create mode 100644 drivers/fpga/microchip-spi.c
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF FPGA manager
2022-03-22 19:15 [PATCH v8 0/2] Microchip Polarfire FPGA manager Ivan Bornyakov
@ 2022-03-22 19:15 ` Ivan Bornyakov
2022-03-30 14:37 ` Conor.Dooley
2022-03-22 19:15 ` [PATCH v8 2/2] dt-bindings: fpga: add binding doc for microchip-spi fpga mgr Ivan Bornyakov
2022-03-30 15:10 ` [PATCH v8 0/2] Microchip Polarfire FPGA manager Xu Yilun
2 siblings, 1 reply; 8+ messages in thread
From: Ivan Bornyakov @ 2022-03-22 19:15 UTC (permalink / raw)
Cc: Ivan Bornyakov, mdf, hao.wu, yilun.xu, trix, conor.dooley,
robh+dt, system, linux-kernel, linux-fpga, devicetree
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>
---
drivers/fpga/Kconfig | 10 +
drivers/fpga/Makefile | 1 +
drivers/fpga/microchip-spi.c | 448 +++++++++++++++++++++++++++++++++++
3 files changed, 459 insertions(+)
create mode 100644 drivers/fpga/microchip-spi.c
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 26025dbab353..791ecf48503a 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -248,4 +248,14 @@ 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
+ select CRC_CCITT
+ 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 4da5273948df..fcb389ca4873 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..5e79677c29f4
--- /dev/null
+++ b/drivers/fpga/microchip-spi.c
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip Polarfire FPGA programming over slave SPI interface.
+ */
+
+#include <linux/crc-ccitt.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)
+
+struct mpf_priv {
+ struct spi_device *spi;
+ bool program_mode;
+};
+
+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;
+ ssize_t status;
+
+ program_mode = priv->program_mode;
+ spi = priv->spi;
+
+ status = spi_w8r8(spi, MPF_SPI_READ_STATUS);
+
+ if (!program_mode && !status)
+ return FPGA_MGR_STATE_OPERATING;
+
+ return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static int poll_status_not_busy(struct spi_device *spi, u8 mask)
+{
+ ssize_t status, timeout = MPF_STATUS_POLL_TIMEOUT;
+
+ while (timeout--) {
+ status = spi_w8r8(spi, MPF_SPI_READ_STATUS);
+ if (status < 0)
+ return status;
+
+ if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
+ return status;
+
+ usleep_range(1000, 2000);
+ }
+
+ 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 get_sgt_data(struct sg_table *sgt, size_t offset, void *data,
+ size_t data_size)
+{
+ struct sg_mapping_iter miter;
+ size_t remainder;
+
+ sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
+ while (sg_miter_next(&miter)) {
+ if (offset < miter.length)
+ break;
+
+ offset -= miter.length;
+ }
+
+ if (!miter.addr) {
+ sg_miter_stop(&miter);
+ return -EFAULT;
+ }
+
+ remainder = max((ssize_t)0, (ssize_t)offset + (ssize_t)data_size -
+ (ssize_t)miter.length);
+
+ memcpy(data, miter.addr + offset, data_size - remainder);
+
+ while (remainder && sg_miter_next(&miter)) {
+ memcpy(data + data_size - remainder, miter.addr,
+ min(remainder, miter.length));
+ remainder = max((ssize_t)0,
+ (ssize_t)remainder - (ssize_t)miter.length);
+ }
+ sg_miter_stop(&miter);
+
+ if (remainder)
+ return -EFAULT;
+
+ return 0;
+}
+
+static ssize_t lookup_block_start(struct sg_table *sgt, int id)
+{
+ size_t block_start_offset, block_id_offset, i;
+ u8 header_size, blocks_num, block_id;
+ u32 block_start;
+ int ret;
+
+ ret = get_sgt_data(sgt, MPF_HEADER_SIZE_OFFSET, &header_size,
+ sizeof(header_size));
+ if (ret)
+ return ret;
+
+ ret = get_sgt_data(sgt, header_size - 1, &blocks_num,
+ sizeof(blocks_num));
+ if (ret)
+ return ret;
+
+ for (i = 0; i < blocks_num; i++) {
+ block_id_offset = header_size +
+ i * MPF_LOOKUP_TABLE_RECORD_SIZE +
+ MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
+
+ ret = get_sgt_data(sgt, block_id_offset, &block_id,
+ sizeof(block_id));
+ if (ret)
+ return ret;
+
+ if (block_id == id) {
+ block_start_offset = header_size +
+ i * MPF_LOOKUP_TABLE_RECORD_SIZE +
+ MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
+
+ ret = get_sgt_data(sgt, block_start_offset, &block_start,
+ sizeof(block_start));
+ if (ret)
+ return ret;
+
+ return le32_to_cpu(block_start);
+ }
+ }
+
+ return -EFAULT;
+}
+
+static ssize_t parse_bitstream_size(struct sg_table *sgt)
+{
+ size_t component_size_byte_num, component_size_byte_off,
+ i, bitstream_size = 0;
+ ssize_t components_size_start;
+ u16 components_num;
+ u32 component_size;
+ int ret;
+
+ ret = get_sgt_data(sgt, MPF_DATA_SIZE_OFFSET, &components_num,
+ sizeof(components_num));
+ if (ret)
+ return ret;
+
+ components_num = le16_to_cpu(components_num);
+
+ components_size_start = lookup_block_start(sgt, MPF_COMPONENTS_SIZE_ID);
+ if (components_size_start < 0)
+ return components_size_start;
+
+ 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;
+
+ ret = get_sgt_data(sgt, components_size_start +
+ component_size_byte_num,
+ &component_size, sizeof(component_size));
+ if (ret)
+ return ret;
+
+ component_size = le32_to_cpu(component_size);
+ component_size >>= component_size_byte_off;
+ component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
+
+ bitstream_size += component_size;
+ }
+
+ return bitstream_size;
+}
+
+static u16 crc_ccitt_sg(struct sg_table *sgt)
+{
+ struct sg_mapping_iter miter;
+ u16 crc = 0;
+
+ sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
+ while (sg_miter_next(&miter))
+ crc = crc_ccitt(crc, miter.addr, miter.length);
+ sg_miter_stop(&miter);
+
+ return crc;
+}
+
+static int mpf_ops_write_sg(struct fpga_manager *mgr, struct sg_table *sgt)
+{
+ u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
+ ssize_t bitstream_start, bitstream_size;
+ struct mpf_priv *priv = mgr->priv;
+ struct device *dev = &mgr->dev;
+ struct sg_mapping_iter miter;
+ struct spi_device *spi;
+ size_t remainder;
+ int ret, i;
+
+ if (crc_ccitt_sg(sgt)) {
+ dev_err(dev, "CRC error\n");
+ return -EINVAL;
+ }
+
+ bitstream_start = lookup_block_start(sgt, MPF_BITSTREAM_ID);
+ if (bitstream_start < 0) {
+ dev_err(dev, "Failed to find bitstream start %zd\n",
+ bitstream_start);
+ return bitstream_start;
+ }
+
+ bitstream_size = parse_bitstream_size(sgt);
+ if (bitstream_size < 0) {
+ dev_err(dev, "Failed to parse bitstream size %zd\n",
+ bitstream_size);
+ return bitstream_size;
+ }
+
+ spi = priv->spi;
+
+ sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
+ for (i = 0; i < bitstream_size; i++) {
+ do {
+ if (bitstream_start < miter.length)
+ break;
+
+ bitstream_start -= miter.length;
+ } while (sg_miter_next(&miter));
+
+ remainder = max((ssize_t)0, bitstream_start +
+ (ssize_t)MPF_SPI_FRAME_SIZE -
+ (ssize_t)miter.length);
+
+ memcpy(tmp_buf + 1, miter.addr + bitstream_start,
+ MPF_SPI_FRAME_SIZE - remainder);
+
+ while (remainder) {
+ bitstream_start -= miter.length;
+
+ if (sg_miter_next(&miter)) {
+ memcpy(tmp_buf + 1 + MPF_SPI_FRAME_SIZE -
+ remainder, miter.addr,
+ min(remainder, miter.length));
+ remainder = max((ssize_t)0, (ssize_t)remainder -
+ (ssize_t)miter.length);
+ } else {
+ dev_err(dev,
+ "Bitstream outruns firmware image. Frame %d of %zd\n",
+ i, bitstream_size);
+ sg_miter_stop(&miter);
+ return -EFAULT;
+ }
+ }
+
+ bitstream_start += MPF_SPI_FRAME_SIZE;
+
+ ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
+ if (ret) {
+ dev_err(dev,
+ "Failed to write bitstream frame number %d of %zd\n",
+ i, bitstream_size);
+ sg_miter_stop(&miter);
+ return ret;
+ }
+ }
+ sg_miter_stop(&miter);
+
+ 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,
+ .write_init = mpf_ops_write_init,
+ .write_sg = mpf_ops_write_sg,
+ .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.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v8 2/2] dt-bindings: fpga: add binding doc for microchip-spi fpga mgr
2022-03-22 19:15 [PATCH v8 0/2] Microchip Polarfire FPGA manager Ivan Bornyakov
2022-03-22 19:15 ` [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF " Ivan Bornyakov
@ 2022-03-22 19:15 ` Ivan Bornyakov
2022-03-29 23:09 ` Rob Herring
2022-03-30 15:10 ` [PATCH v8 0/2] Microchip Polarfire FPGA manager Xu Yilun
2 siblings, 1 reply; 8+ messages in thread
From: Ivan Bornyakov @ 2022-03-22 19:15 UTC (permalink / raw)
Cc: Ivan Bornyakov, mdf, hao.wu, yilun.xu, trix, conor.dooley,
robh+dt, system, linux-kernel, linux-fpga, devicetree
Add Device Tree Binding doc for Microchip Polarfire FPGA Manager using
slave SPI to load .dat formatted bitstream image.
Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
---
.../fpga/microchip,mpf-spi-fpga-mgr.yaml | 44 +++++++++++++++++++
1 file changed, 44 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
diff --git a/Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml b/Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
new file mode 100644
index 000000000000..aee45cb15592
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/microchip,mpf-spi-fpga-mgr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip Polarfire FPGA manager.
+
+maintainers:
+ - Ivan Bornyakov <i.bornyakov@metrotek.ru>
+
+description:
+ Device Tree Bindings for Microchip Polarfire FPGA Manager using slave SPI to
+ load the bitstream in .dat format.
+
+properties:
+ compatible:
+ enum:
+ - microchip,mpf-spi-fpga-mgr
+
+ reg:
+ description: SPI chip select
+ maxItems: 1
+
+ spi-max-frequency: true
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ fpga_mgr@0 {
+ compatible = "microchip,mpf-spi-fpga-mgr";
+ spi-max-frequency = <20000000>;
+ reg = <0>;
+ };
+ };
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v8 2/2] dt-bindings: fpga: add binding doc for microchip-spi fpga mgr
2022-03-22 19:15 ` [PATCH v8 2/2] dt-bindings: fpga: add binding doc for microchip-spi fpga mgr Ivan Bornyakov
@ 2022-03-29 23:09 ` Rob Herring
0 siblings, 0 replies; 8+ messages in thread
From: Rob Herring @ 2022-03-29 23:09 UTC (permalink / raw)
To: Ivan Bornyakov
Cc: yilun.xu, conor.dooley, trix, devicetree, hao.wu, linux-fpga,
robh+dt, system, mdf, linux-kernel
On Tue, 22 Mar 2022 22:15:52 +0300, Ivan Bornyakov wrote:
> Add Device Tree Binding doc for Microchip Polarfire FPGA Manager using
> slave SPI to load .dat formatted bitstream image.
>
> Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> ---
> .../fpga/microchip,mpf-spi-fpga-mgr.yaml | 44 +++++++++++++++++++
> 1 file changed, 44 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF FPGA manager
2022-03-22 19:15 ` [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF " Ivan Bornyakov
@ 2022-03-30 14:37 ` Conor.Dooley
2022-03-30 15:48 ` Ivan Bornyakov
0 siblings, 1 reply; 8+ messages in thread
From: Conor.Dooley @ 2022-03-30 14:37 UTC (permalink / raw)
To: i.bornyakov
Cc: mdf, hao.wu, yilun.xu, trix, robh+dt, system, linux-kernel,
linux-fpga, devicetree, Cyril.Jean
Hey Ivan,
Been testing this and generated a couple questions.
I've put them inline where they were relevant.
Thanks,
Conor.
On 22/03/2022 19:15, 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>
> ---
> drivers/fpga/Kconfig | 10 +
> drivers/fpga/Makefile | 1 +
> drivers/fpga/microchip-spi.c | 448 +++++++++++++++++++++++++++++++++++
> 3 files changed, 459 insertions(+)
> create mode 100644 drivers/fpga/microchip-spi.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 26025dbab353..791ecf48503a 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -248,4 +248,14 @@ 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
> + select CRC_CCITT
> + 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 4da5273948df..fcb389ca4873 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..5e79677c29f4
> --- /dev/null
> +++ b/drivers/fpga/microchip-spi.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip Polarfire FPGA programming over slave SPI interface.
> + */
> +
> +#include <linux/crc-ccitt.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)
> +
> +struct mpf_priv {
> + struct spi_device *spi;
> + bool program_mode;
> +};
> +
> +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;
> + ssize_t status;
> +
> + program_mode = priv->program_mode;
> + spi = priv->spi;
> +
> + status = spi_w8r8(spi, MPF_SPI_READ_STATUS);
> +
> + if (!program_mode && !status)
> + return FPGA_MGR_STATE_OPERATING;
> +
> + return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> +{
> + ssize_t status, timeout = MPF_STATUS_POLL_TIMEOUT;
> +
> + while (timeout--) {
> + status = spi_w8r8(spi, MPF_SPI_READ_STATUS);> + if (status < 0)
> + return status;
> +
> + if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
> + return status;
> +
> + usleep_range(1000, 2000);
> + }
> +
> + 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;
> + }
So, my test board for this has had a PolarFire SoC, not a standard
PolarFire. I ran into some problems with the ISC enable code, due to
a sequence error. After sending the SPI_ISC_ENABLE, you then do a
poll_status_not_busy to hold until you see a STATUS_READY.
poll_status_not_busy does a w8r8 to request and then read the status,
and you expect a sequence as below:
op: w w r w r
M: 0xB 0x0 0x0
S: 0x1 0x2
I could not get past this check & it would just poll until the
timeout. What I saw on a protocol analyser was more like so:
op: w w r w r
M: 0xB 0x0 0x0
S: 0x1 0x0 0x2 0x0
So the read in that w8r8 would always get a zero back and then time out.
Changing the poll function (just for isc) to only read gave:
op: w r r
M: 0xB 0x0 0x0
S: 0x1 0x2
For the code after the ISC enable, I reverted to your implementation
of the poll function & the rest of the programming sequence ran.
I spoke to the guys that wrote the HW about this, and they said that
reading the status back *as* the 0x0 the poll command is clocked in is
the expected behaviour.
They also said that MPF should work identically to an MPFS and I was unable
to find a documented difference between MPF and MPFS other than the envm,
which is an optional component anyway.
But I can only assume that what you were doing worked for you, so if
you could possibly share some waveforms of the write_init sequence
that'd be great. Or if there is something that you think I am
overlooking, please let me know.
> +
> + 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 get_sgt_data(struct sg_table *sgt, size_t offset, void *data,
> + size_t data_size)
> +{
> + struct sg_mapping_iter miter;
> + size_t remainder;
> +
> + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
> + while (sg_miter_next(&miter)) {
> + if (offset < miter.length)
> + break;
> +
> + offset -= miter.length;
> + }
> +
> + if (!miter.addr) {
> + sg_miter_stop(&miter);
> + return -EFAULT;
> + }
> +
> + remainder = max((ssize_t)0, (ssize_t)offset + (ssize_t)data_size -
> + (ssize_t)miter.length);
> +
> + memcpy(data, miter.addr + offset, data_size - remainder);
> +
> + while (remainder && sg_miter_next(&miter)) {
> + memcpy(data + data_size - remainder, miter.addr,
> + min(remainder, miter.length));
> + remainder = max((ssize_t)0,
> + (ssize_t)remainder - (ssize_t)miter.length);
> + }
> + sg_miter_stop(&miter);
> +
> + if (remainder)
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static ssize_t lookup_block_start(struct sg_table *sgt, int id)
> +{
> + size_t block_start_offset, block_id_offset, i;
> + u8 header_size, blocks_num, block_id;
> + u32 block_start;
> + int ret;
> +
> + ret = get_sgt_data(sgt, MPF_HEADER_SIZE_OFFSET, &header_size,
> + sizeof(header_size));
> + if (ret)
> + return ret;
> +
> + ret = get_sgt_data(sgt, header_size - 1, &blocks_num,
> + sizeof(blocks_num));
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < blocks_num; i++) {
> + block_id_offset = header_size +
> + i * MPF_LOOKUP_TABLE_RECORD_SIZE +
> + MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
> +
> + ret = get_sgt_data(sgt, block_id_offset, &block_id,
> + sizeof(block_id));
> + if (ret)
> + return ret;
> +
> + if (block_id == id) {
> + block_start_offset = header_size +
> + i * MPF_LOOKUP_TABLE_RECORD_SIZE +
> + MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
> +
> + ret = get_sgt_data(sgt, block_start_offset, &block_start,
> + sizeof(block_start));
> + if (ret)
> + return ret;
> +
> + return le32_to_cpu(block_start);
> + }
> + }
> +
> + return -EFAULT;
> +}
> +
> +static ssize_t parse_bitstream_size(struct sg_table *sgt)
> +{
> + size_t component_size_byte_num, component_size_byte_off,
> + i, bitstream_size = 0;
> + ssize_t components_size_start;
> + u16 components_num;
> + u32 component_size;
> + int ret;
> +
> + ret = get_sgt_data(sgt, MPF_DATA_SIZE_OFFSET, &components_num,
> + sizeof(components_num));
> + if (ret)
> + return ret;
> +
> + components_num = le16_to_cpu(components_num);
Not expecting you to do this at all since you don't have one, but this
will need to be extended for polarfire soc since components_* doesn't
include the envm which has its own parameter.
> +
> + components_size_start = lookup_block_start(sgt, MPF_COMPONENTS_SIZE_ID);
> + if (components_size_start < 0)
> + return components_size_start;
> +
> + 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;
> +
> + ret = get_sgt_data(sgt, components_size_start +
> + component_size_byte_num,
> + &component_size, sizeof(component_size));
> + if (ret)
> + return ret;
> +
> + component_size = le32_to_cpu(component_size);
> + component_size >>= component_size_byte_off;
> + component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
> +
> + bitstream_size += component_size;
> + }
> +
> + return bitstream_size;
> +}
> +
> +static u16 crc_ccitt_sg(struct sg_table *sgt)
> +{
> + struct sg_mapping_iter miter;
> + u16 crc = 0;
> +
> + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
> + while (sg_miter_next(&miter))
> + crc = crc_ccitt(crc, miter.addr, miter.length);
> + sg_miter_stop(&miter);
> +
> + return crc;
> +}
> +
> +static int mpf_ops_write_sg(struct fpga_manager *mgr, struct sg_table *sgt)
> +{
> + u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
> + ssize_t bitstream_start, bitstream_size;
> + struct mpf_priv *priv = mgr->priv;
> + struct device *dev = &mgr->dev;
> + struct sg_mapping_iter miter;
> + struct spi_device *spi;
> + size_t remainder;
> + int ret, i;
> +
> + if (crc_ccitt_sg(sgt)) {
> + dev_err(dev, "CRC error\n");
> + return -EINVAL;
> + }
> +
> + bitstream_start = lookup_block_start(sgt, MPF_BITSTREAM_ID);
> + if (bitstream_start < 0) {
> + dev_err(dev, "Failed to find bitstream start %zd\n",
> + bitstream_start);
> + return bitstream_start;
> + }
> +
> + bitstream_size = parse_bitstream_size(sgt);
> + if (bitstream_size < 0) {
> + dev_err(dev, "Failed to parse bitstream size %zd\n",
> + bitstream_size);
> + return bitstream_size;
> + }
> +
> + spi = priv->spi;
> +
> + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);> + for (i = 0; i < bitstream_size; i++) {
> + do {
> + if (bitstream_start < miter.length)
> + break;
> +
> + bitstream_start -= miter.length;
> + } while (sg_miter_next(&miter));
> +
> + remainder = max((ssize_t)0, bitstream_start +
> + (ssize_t)MPF_SPI_FRAME_SIZE -
> + (ssize_t)miter.length);
> +
> + memcpy(tmp_buf + 1, miter.addr + bitstream_start,
> + MPF_SPI_FRAME_SIZE - remainder);
> +
> + while (remainder) {
> + bitstream_start -= miter.length;
> +
> + if (sg_miter_next(&miter)) {
> + memcpy(tmp_buf + 1 + MPF_SPI_FRAME_SIZE -
> + remainder, miter.addr,
> + min(remainder, miter.length));
> + remainder = max((ssize_t)0, (ssize_t)remainder -
> + (ssize_t)miter.length);
> + } else {
> + dev_err(dev,
> + "Bitstream outruns firmware image. Frame %d of %zd\n",
> + i, bitstream_size);
> + sg_miter_stop(&miter);
> + return -EFAULT;
> + }
> + }
> +
> + bitstream_start += MPF_SPI_FRAME_SIZE;
> +
> + ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
> + if (ret) {
> + dev_err(dev,
> + "Failed to write bitstream frame number %d of %zd\n",
> + i, bitstream_size);
> + sg_miter_stop(&miter);
> + return ret;
> + }
> + }
> + sg_miter_stop(&miter);
> +
> + 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;
> + }
While I was debugging the sequence issue, it got to this stage of the
transfer several times, but hadn't actually programmed the device.
It seems that poll_status_not_busy covers checking ERR/VIOLATION from
polling the status, but again that check is avoided due to the
sequence issues I mentioned above.
It might be nice to check the device certificate, but I am not sure
if that fits the flow for fpga managers.
> +
> + priv->program_mode = false;
> +
> + return 0;
> +}
> +
> +static const struct fpga_manager_ops mpf_ops = {
> + .state = mpf_ops_state,
> + .write_init = mpf_ops_write_init,
> + .write_sg = mpf_ops_write_sg,
> + .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.34.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 0/2] Microchip Polarfire FPGA manager
2022-03-22 19:15 [PATCH v8 0/2] Microchip Polarfire FPGA manager Ivan Bornyakov
2022-03-22 19:15 ` [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF " Ivan Bornyakov
2022-03-22 19:15 ` [PATCH v8 2/2] dt-bindings: fpga: add binding doc for microchip-spi fpga mgr Ivan Bornyakov
@ 2022-03-30 15:10 ` Xu Yilun
2 siblings, 0 replies; 8+ messages in thread
From: Xu Yilun @ 2022-03-30 15:10 UTC (permalink / raw)
To: Ivan Bornyakov
Cc: mdf, hao.wu, trix, conor.dooley, robh+dt, system, linux-kernel,
linux-fpga, devicetree
On Tue, Mar 22, 2022 at 10:15:50PM +0300, Ivan Bornyakov wrote:
> Add support to the FPGA manager for programming Microchip Polarfire
> FPGAs over slave SPI interface with .dat formatted bitsream image.
>
> Changelog:
> v1 -> v2: fix printk formating
> v2 -> v3:
> * replace "microsemi" with "microchip"
> * replace prefix "microsemi_fpga_" with "mpf_"
> * more sensible .compatible and .name strings
> * remove unused defines STATUS_SPI_VIOLATION and STATUS_SPI_ERROR
> v3 -> v4: fix unused variable warning
> Put 'mpf_of_ids' definition under conditional compilation, so it
> would not hang unused if CONFIG_OF is not enabled.
> v4 -> v5:
> * prefix defines with MPF_
> * mdelay() -> usleep_range()
> * formatting fixes
> * add DT bindings doc
> * rework fpga_manager_ops.write() to fpga_manager_ops.write_sg()
> We can't parse image header in write_init() because image header
> size is not known beforehand. Thus parsing need to be done in
> fpga_manager_ops.write() callback, but fpga_manager_ops.write()
> also need to be reenterable. On the other hand,
> fpga_manager_ops.write_sg() is called once. Thus, rework usage of
> write() callback to write_sg().
Sorry for late reply.
write_sg is for hardwares capable of handling sg buffers in batches. It
does not make sense we must implement write_sg() if the image header is
not fixed. They are not related.
The real problem is we lack the support for variable image header. So
maybe add the support in fpga manager core.
Thanks,
Yilun
> v5 -> v6: fix patch applying
> I forgot to clean up unrelated local changes which lead to error on
> patch 0001-fpga-microchip-spi-add-Microchip-MPF-FPGA-manager.patch
> applying on vanilla kernel.
> v6 -> v7: fix binding doc to pass dt_binding_check
> v7 -> v8: another fix for dt_binding_check warning
>
> Ivan Bornyakov (2):
> fpga: microchip-spi: add Microchip MPF FPGA manager
> dt-bindings: fpga: add binding doc for microchip-spi fpga mgr
>
> .../fpga/microchip,mpf-spi-fpga-mgr.yaml | 44 ++
> drivers/fpga/Kconfig | 10 +
> drivers/fpga/Makefile | 1 +
> drivers/fpga/microchip-spi.c | 448 ++++++++++++++++++
> 4 files changed, 503 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
> create mode 100644 drivers/fpga/microchip-spi.c
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF FPGA manager
2022-03-30 14:37 ` Conor.Dooley
@ 2022-03-30 15:48 ` Ivan Bornyakov
2022-03-31 9:13 ` Conor.Dooley
0 siblings, 1 reply; 8+ messages in thread
From: Ivan Bornyakov @ 2022-03-30 15:48 UTC (permalink / raw)
To: Conor.Dooley
Cc: mdf, hao.wu, yilun.xu, trix, robh+dt, system, linux-kernel,
linux-fpga, devicetree, Cyril.Jean
Hi, Conor!
On Wed, Mar 30, 2022 at 02:37:05PM +0000, Conor.Dooley@microchip.com wrote:
> Hey Ivan,
> Been testing this and generated a couple questions.
> I've put them inline where they were relevant.
> Thanks,
> Conor.
>
> On 22/03/2022 19:15, Ivan Bornyakov wrote:
> >
> > ... snip ...
> >
> > +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;
> > + }
>
> So, my test board for this has had a PolarFire SoC, not a standard
> PolarFire. I ran into some problems with the ISC enable code, due to
> a sequence error. After sending the SPI_ISC_ENABLE, you then do a
> poll_status_not_busy to hold until you see a STATUS_READY.
> poll_status_not_busy does a w8r8 to request and then read the status,
> and you expect a sequence as below:
>
> op: w w r w r
> M: 0xB 0x0 0x0
> S: 0x1 0x2
>
> I could not get past this check & it would just poll until the
> timeout. What I saw on a protocol analyser was more like so:
>
> op: w w r w r
> M: 0xB 0x0 0x0
> S: 0x1 0x0 0x2 0x0
>
> So the read in that w8r8 would always get a zero back and then time out.
> Changing the poll function (just for isc) to only read gave:
>
> op: w r r
> M: 0xB 0x0 0x0
> S: 0x1 0x2
>
> For the code after the ISC enable, I reverted to your implementation
> of the poll function & the rest of the programming sequence ran.
>
> I spoke to the guys that wrote the HW about this, and they said that
> reading the status back *as* the 0x0 the poll command is clocked in is
> the expected behaviour.
> They also said that MPF should work identically to an MPFS and I was unable
> to find a documented difference between MPF and MPFS other than the envm,
> which is an optional component anyway.
>
> But I can only assume that what you were doing worked for you, so if
> you could possibly share some waveforms of the write_init sequence
> that'd be great. Or if there is something that you think I am
> overlooking, please let me know.
>
If you replace poll_status_not_busy() function with this code:
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;
}
Will it work for you? It is still works in my case.
> >
> > ... snip ...
> >
> > +static ssize_t parse_bitstream_size(struct sg_table *sgt)
> > +{
> > + size_t component_size_byte_num, component_size_byte_off,
> > + i, bitstream_size = 0;
> > + ssize_t components_size_start;
> > + u16 components_num;
> > + u32 component_size;
> > + int ret;
> > +
> > + ret = get_sgt_data(sgt, MPF_DATA_SIZE_OFFSET, &components_num,
> > + sizeof(components_num));
> > + if (ret)
> > + return ret;
> > +
> > + components_num = le16_to_cpu(components_num);
>
> Not expecting you to do this at all since you don't have one, but this
> will need to be extended for polarfire soc since components_* doesn't
> include the envm which has its own parameter.
>
> > +
> > + components_size_start = lookup_block_start(sgt, MPF_COMPONENTS_SIZE_ID);
> > + if (components_size_start < 0)
> > + return components_size_start;
> > +
> > + 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;
> > +
> > + ret = get_sgt_data(sgt, components_size_start +
> > + component_size_byte_num,
> > + &component_size, sizeof(component_size));
> > + if (ret)
> > + return ret;
> > +
> > + component_size = le32_to_cpu(component_size);
> > + component_size >>= component_size_byte_off;
> > + component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
> > +
> > + bitstream_size += component_size;
> > + }
> > +
> > + return bitstream_size;
> > +}
> >
> > ... snip ...
> >
> > +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;
> > + }
>
> While I was debugging the sequence issue, it got to this stage of the
> transfer several times, but hadn't actually programmed the device.
> It seems that poll_status_not_busy covers checking ERR/VIOLATION from
> polling the status, but again that check is avoided due to the
> sequence issues I mentioned above.
>
> It might be nice to check the device certificate, but I am not sure
> if that fits the flow for fpga managers.
>
Noted.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF FPGA manager
2022-03-30 15:48 ` Ivan Bornyakov
@ 2022-03-31 9:13 ` Conor.Dooley
0 siblings, 0 replies; 8+ messages in thread
From: Conor.Dooley @ 2022-03-31 9:13 UTC (permalink / raw)
To: i.bornyakov
Cc: mdf, hao.wu, yilun.xu, trix, robh+dt, system, linux-kernel,
linux-fpga, devicetree, Cyril.Jean
On 30/03/2022 15:48, Ivan Bornyakov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi, Conor!
>
> On Wed, Mar 30, 2022 at 02:37:05PM +0000, Conor.Dooley@microchip.com wrote:
>> Hey Ivan,
>> Been testing this and generated a couple questions.
>> I've put them inline where they were relevant.
>> Thanks,
>> Conor.
>>
>> On 22/03/2022 19:15, Ivan Bornyakov wrote:
>>>
>>> ... snip ...
>>>
>>> +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;
>>> + }
>>
>> So, my test board for this has had a PolarFire SoC, not a standard
>> PolarFire. I ran into some problems with the ISC enable code, due to
>> a sequence error. After sending the SPI_ISC_ENABLE, you then do a
>> poll_status_not_busy to hold until you see a STATUS_READY.
>> poll_status_not_busy does a w8r8 to request and then read the status,
>> and you expect a sequence as below:
>>
>> op: w w r w r
>> M: 0xB 0x0 0x0
>> S: 0x1 0x2
>>
>> I could not get past this check & it would just poll until the
>> timeout. What I saw on a protocol analyser was more like so:
>>
>> op: w w r w r
>> M: 0xB 0x0 0x0
>> S: 0x1 0x0 0x2 0x0
>>
>> So the read in that w8r8 would always get a zero back and then time out.
>> Changing the poll function (just for isc) to only read gave:
>>
>> op: w r r
>> M: 0xB 0x0 0x0
>> S: 0x1 0x2
>>
>> For the code after the ISC enable, I reverted to your implementation
>> of the poll function & the rest of the programming sequence ran.
>>
>> I spoke to the guys that wrote the HW about this, and they said that
>> reading the status back *as* the 0x0 the poll command is clocked in is
>> the expected behaviour.
>> They also said that MPF should work identically to an MPFS and I was unable
>> to find a documented difference between MPF and MPFS other than the envm,
>> which is an optional component anyway.
>>
>> But I can only assume that what you were doing worked for you, so if
>> you could possibly share some waveforms of the write_init sequence
>> that'd be great. Or if there is something that you think I am
>> overlooking, please let me know.
>>
>
> If you replace poll_status_not_busy() function with this code:
>
> 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;
> }
>
> Will it work for you? It is still works in my case.
Yeah, status checking after the ISC enable works for me with
this changed. However, the mpf_ops_state still uses w8r8 &
will need to be changed too.
Thanks,
Conor.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-03-31 9:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-22 19:15 [PATCH v8 0/2] Microchip Polarfire FPGA manager Ivan Bornyakov
2022-03-22 19:15 ` [PATCH v8 1/2] fpga: microchip-spi: add Microchip MPF " Ivan Bornyakov
2022-03-30 14:37 ` Conor.Dooley
2022-03-30 15:48 ` Ivan Bornyakov
2022-03-31 9:13 ` Conor.Dooley
2022-03-22 19:15 ` [PATCH v8 2/2] dt-bindings: fpga: add binding doc for microchip-spi fpga mgr Ivan Bornyakov
2022-03-29 23:09 ` Rob Herring
2022-03-30 15:10 ` [PATCH v8 0/2] Microchip Polarfire FPGA manager Xu Yilun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).