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 | 370 +++++++++++++++++++++++++++++++++++
3 files changed, 380 insertions(+)
create mode 100644 drivers/fpga/microchip-spi.c
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 26025dbab353..75806ef5c9ea 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -248,4 +248,13 @@ config FPGA_MGR_VERSAL_FPGA
configure the programmable logic(PL).
To compile this as a module, choose M here.
+
+config FPGA_MGR_MICROCHIP_SPI
+ tristate "Microchip Polarfire SPI FPGA manager"
+ depends on SPI
+ help
+ FPGA manager driver support for Microchip Polarfire FPGAs
+ programming over slave SPI interface with .dat formatted
+ bitstream image.
+
endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index e32bfa90f968..5425a15892df 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI) += xilinx-spi.o
obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += zynqmp-fpga.o
obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA) += versal-fpga.o
+obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI) += microchip-spi.o
obj-$(CONFIG_ALTERA_PR_IP_CORE) += altera-pr-ip-core.o
obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT) += altera-pr-ip-core-plat.o
diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
new file mode 100644
index 000000000000..182471c88018
--- /dev/null
+++ b/drivers/fpga/microchip-spi.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip Polarfire FPGA programming over slave SPI interface.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+
+#define MPF_SPI_ISC_ENABLE 0x0B
+#define MPF_SPI_ISC_DISABLE 0x0C
+#define MPF_SPI_READ_STATUS 0x00
+#define MPF_SPI_READ_DATA 0x01
+#define MPF_SPI_FRAME_INIT 0xAE
+#define MPF_SPI_FRAME 0xEE
+#define MPF_SPI_PRG_MODE 0x01
+#define MPF_SPI_RELEASE 0x23
+
+#define MPF_SPI_FRAME_SIZE 16
+
+#define MPF_HEADER_SIZE_OFFSET 24
+#define MPF_DATA_SIZE_OFFSET 55
+
+#define MPF_LOOKUP_TABLE_RECORD_SIZE 9
+#define MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET 0
+#define MPF_LOOKUP_TABLE_BLOCK_START_OFFSET 1
+
+#define MPF_COMPONENTS_SIZE_ID 5
+#define MPF_BITSTREAM_ID 8
+
+#define MPF_BITS_PER_COMPONENT_SIZE 22
+
+#define MPF_STATUS_POLL_TIMEOUT 1000
+#define MPF_STATUS_BUSY BIT(0)
+#define MPF_STATUS_READY BIT(1)
+#define MPF_STATUS_SPI_VIOLATION BIT(2)
+#define MPF_STATUS_SPI_ERROR BIT(3)
+
+struct mpf_priv {
+ struct spi_device *spi;
+ bool program_mode;
+};
+
+static int mpf_read_status(struct spi_device *spi)
+{
+ u8 status, status_command = MPF_SPI_READ_STATUS;
+ struct spi_transfer xfer = {
+ .tx_buf = &status_command,
+ .rx_buf = &status,
+ .len = 1,
+ };
+ int ret = spi_sync_transfer(spi, &xfer, 1);
+
+ if ((status & MPF_STATUS_SPI_VIOLATION) ||
+ (status & MPF_STATUS_SPI_ERROR))
+ ret = -EIO;
+
+ return ret ? : status;
+}
+
+static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr)
+{
+ struct mpf_priv *priv = mgr->priv;
+ struct spi_device *spi;
+ bool program_mode;
+ int status;
+
+ spi = priv->spi;
+ program_mode = priv->program_mode;
+ status = mpf_read_status(spi);
+
+ if (!program_mode && !status)
+ return FPGA_MGR_STATE_OPERATING;
+
+ return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static int mpf_ops_parse_header(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *buf, size_t count)
+{
+ size_t component_size_byte_num, component_size_byte_off,
+ components_size_start = 0, bitstream_start = 0,
+ block_id_offset, block_start_offset, i;
+ u8 header_size, blocks_num, block_id;
+ u32 block_start, component_size;
+ u16 components_num;
+
+ if (!buf) {
+ dev_err(&mgr->dev, "Image buffer is not provided\n");
+ return -EINVAL;
+ }
+
+ header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
+ if (header_size > count) {
+ info->header_size = header_size;
+ return -EAGAIN;
+ }
+
+ /*
+ * Go through look-up table to find out where actual bitstream starts
+ * and where sizes of components of the bitstream lies.
+ */
+ blocks_num = *(buf + header_size - 1);
+ block_id_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
+ block_start_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
+
+ header_size += blocks_num * MPF_LOOKUP_TABLE_RECORD_SIZE;
+ if (header_size > count) {
+ info->header_size = header_size;
+ return -EAGAIN;
+ }
+
+ while (blocks_num--) {
+ block_id = *(buf + block_id_offset);
+ block_start = get_unaligned_le32(buf + block_start_offset);
+
+ switch (block_id) {
+ case MPF_BITSTREAM_ID:
+ info->header_size = bitstream_start = block_start;
+ if (block_start > count)
+ return -EAGAIN;
+
+ break;
+ case MPF_COMPONENTS_SIZE_ID:
+ components_size_start = block_start;
+ break;
+ default:
+ break;
+ }
+
+ if (bitstream_start && components_size_start)
+ break;
+
+ block_id_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
+ block_start_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
+ }
+
+ if (!bitstream_start || !components_size_start) {
+ dev_err(&mgr->dev, "Failed to parse header look-up table\n");
+ return -EFAULT;
+ }
+
+ /*
+ * Parse bitstream size.
+ * Sizes of components of the bitstream are 22-bits long placed next
+ * to each other. Image header should be extended by now up to where
+ * actual bitstream starts, so no need for overflow check anymore.
+ */
+ components_num = get_unaligned_le16(buf + MPF_DATA_SIZE_OFFSET);
+
+ for (i = 0; i < components_num; i++) {
+ component_size_byte_num =
+ (i * MPF_BITS_PER_COMPONENT_SIZE) / BITS_PER_BYTE;
+ component_size_byte_off =
+ (i * MPF_BITS_PER_COMPONENT_SIZE) % BITS_PER_BYTE;
+
+ component_size = get_unaligned_le32(buf +
+ components_size_start +
+ component_size_byte_num);
+ component_size >>= component_size_byte_off;
+ component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
+
+ info->data_size += component_size * MPF_SPI_FRAME_SIZE;
+ }
+
+ return 0;
+}
+
+static int poll_status_not_busy(struct spi_device *spi, u8 mask)
+{
+ int status, timeout = MPF_STATUS_POLL_TIMEOUT;
+
+ while (timeout--) {
+ status = mpf_read_status(spi);
+ if (status < 0 ||
+ (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
+ return status;
+
+ usleep_range(1000, 2000);
+ }
+
+ return -EBUSY;
+}
+
+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
Hey Ivan, one comment below.
Thanks,
Conor.
On 07/05/2022 08:43, Ivan Bornyakov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Add support to the FPGA manager for programming Microchip Polarfire
> FPGAs over slave SPI interface with .dat formatted bitsream image.
>
> Signed-off-by: Ivan Bornyakov <[email protected]>
> ---
> drivers/fpga/Kconfig | 9 +
> drivers/fpga/Makefile | 1 +
> drivers/fpga/microchip-spi.c | 370 +++++++++++++++++++++++++++++++++++
> 3 files changed, 380 insertions(+)
> create mode 100644 drivers/fpga/microchip-spi.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 26025dbab353..75806ef5c9ea 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -248,4 +248,13 @@ config FPGA_MGR_VERSAL_FPGA
> configure the programmable logic(PL).
>
> To compile this as a module, choose M here.
> +
> +config FPGA_MGR_MICROCHIP_SPI
> + tristate "Microchip Polarfire SPI FPGA manager"
> + depends on SPI
> + help
> + FPGA manager driver support for Microchip Polarfire FPGAs
> + programming over slave SPI interface with .dat formatted
> + bitstream image.
> +
> endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index e32bfa90f968..5425a15892df 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI) += xilinx-spi.o
> obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += zynqmp-fpga.o
> obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA) += versal-fpga.o
> +obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI) += microchip-spi.o
> obj-$(CONFIG_ALTERA_PR_IP_CORE) += altera-pr-ip-core.o
> obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT) += altera-pr-ip-core-plat.o
>
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> new file mode 100644
> index 000000000000..182471c88018
> --- /dev/null
> +++ b/drivers/fpga/microchip-spi.c
> @@ -0,0 +1,370 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip Polarfire FPGA programming over slave SPI interface.
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +
> +#define MPF_SPI_ISC_ENABLE 0x0B
> +#define MPF_SPI_ISC_DISABLE 0x0C
> +#define MPF_SPI_READ_STATUS 0x00
> +#define MPF_SPI_READ_DATA 0x01
> +#define MPF_SPI_FRAME_INIT 0xAE
> +#define MPF_SPI_FRAME 0xEE
> +#define MPF_SPI_PRG_MODE 0x01
> +#define MPF_SPI_RELEASE 0x23
> +
> +#define MPF_SPI_FRAME_SIZE 16
> +
> +#define MPF_HEADER_SIZE_OFFSET 24
> +#define MPF_DATA_SIZE_OFFSET 55
> +
> +#define MPF_LOOKUP_TABLE_RECORD_SIZE 9
> +#define MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET 0
> +#define MPF_LOOKUP_TABLE_BLOCK_START_OFFSET 1
> +
> +#define MPF_COMPONENTS_SIZE_ID 5
> +#define MPF_BITSTREAM_ID 8
> +
> +#define MPF_BITS_PER_COMPONENT_SIZE 22
> +
> +#define MPF_STATUS_POLL_TIMEOUT 1000
> +#define MPF_STATUS_BUSY BIT(0)
> +#define MPF_STATUS_READY BIT(1)
> +#define MPF_STATUS_SPI_VIOLATION BIT(2)
> +#define MPF_STATUS_SPI_ERROR BIT(3)
> +
> +struct mpf_priv {
> + struct spi_device *spi;
> + bool program_mode;
> +};
> +
> +static int mpf_read_status(struct spi_device *spi)
> +{
> + u8 status, status_command = MPF_SPI_READ_STATUS;
> + struct spi_transfer xfer = {
> + .tx_buf = &status_command,
> + .rx_buf = &status,
> + .len = 1,
> + };
> + int ret = spi_sync_transfer(spi, &xfer, 1);
> +
> + if ((status & MPF_STATUS_SPI_VIOLATION) ||
> + (status & MPF_STATUS_SPI_ERROR))
> + ret = -EIO;
> +
> + return ret ? : status;
> +}
> +
> +static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr)
> +{
> + struct mpf_priv *priv = mgr->priv;
> + struct spi_device *spi;
> + bool program_mode;
> + int status;
> +
> + spi = priv->spi;
> + program_mode = priv->program_mode;
> + status = mpf_read_status(spi);
> +
> + if (!program_mode && !status)
> + return FPGA_MGR_STATE_OPERATING;
> +
> + return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int mpf_ops_parse_header(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + size_t component_size_byte_num, component_size_byte_off,
> + components_size_start = 0, bitstream_start = 0,
> + block_id_offset, block_start_offset, i;
> + u8 header_size, blocks_num, block_id;
> + u32 block_start, component_size;
> + u16 components_num;
> +
> + if (!buf) {
> + dev_err(&mgr->dev, "Image buffer is not provided\n");
> + return -EINVAL;
> + }
> +
> + header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
> + if (header_size > count) {
> + info->header_size = header_size;
> + return -EAGAIN;
> + }
> +
> + /*
> + * Go through look-up table to find out where actual bitstream starts
> + * and where sizes of components of the bitstream lies.
> + */
> + blocks_num = *(buf + header_size - 1);
> + block_id_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
> + block_start_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
> +
> + header_size += blocks_num * MPF_LOOKUP_TABLE_RECORD_SIZE;
> + if (header_size > count) {
> + info->header_size = header_size;
> + return -EAGAIN;
> + }
> +
> + while (blocks_num--) {
> + block_id = *(buf + block_id_offset);
> + block_start = get_unaligned_le32(buf + block_start_offset);
> +
> + switch (block_id) {
> + case MPF_BITSTREAM_ID:
> + info->header_size = bitstream_start = block_start;
> + if (block_start > count)
> + return -EAGAIN;
> +
> + break;
> + case MPF_COMPONENTS_SIZE_ID:
> + components_size_start = block_start;
> + break;
> + default:
> + break;
> + }
> +
> + if (bitstream_start && components_size_start)
> + break;
> +
> + block_id_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
> + block_start_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
> + }
> +
> + if (!bitstream_start || !components_size_start) {
> + dev_err(&mgr->dev, "Failed to parse header look-up table\n");
> + return -EFAULT;
> + }
> +
> + /*
> + * Parse bitstream size.
> + * Sizes of components of the bitstream are 22-bits long placed next
> + * to each other. Image header should be extended by now up to where
> + * actual bitstream starts, so no need for overflow check anymore.
> + */
> + components_num = get_unaligned_le16(buf + MPF_DATA_SIZE_OFFSET);
> +
> + for (i = 0; i < components_num; i++) {
> + component_size_byte_num =
> + (i * MPF_BITS_PER_COMPONENT_SIZE) / BITS_PER_BYTE;
> + component_size_byte_off =
> + (i * MPF_BITS_PER_COMPONENT_SIZE) % BITS_PER_BYTE;
> +
> + component_size = get_unaligned_le32(buf +
> + components_size_start +
> + component_size_byte_num);
> + component_size >>= component_size_byte_off;
> + component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
> +
> + info->data_size += component_size * MPF_SPI_FRAME_SIZE;
> + }
> +
> + return 0;
> +}
> +
> +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> +{
> + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> +
> + while (timeout--) {
> + status = mpf_read_status(spi);
> + if (status < 0 ||
> + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> + return status;
> +
> + usleep_range(1000, 2000);
> + }
> +
> + return -EBUSY;
> +}
Is there a reason you changed this from the snippet you sent me
in the responses to version 8:
static int poll_status_not_busy(struct spi_device *spi, u8 mask)
{
u8 status, status_command = MPF_SPI_READ_STATUS;
int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
struct spi_transfer xfer = {
.tx_buf = &status_command,
.rx_buf = &status,
.len = 1,
};
while (timeout--) {
ret = spi_sync_transfer(spi, &xfer, 1);
if (ret < 0)
return ret;
if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
return status;
usleep_range(1000, 2000);
}
return -EBUSY;
}
With the current version, I hit the "Failed to write bitstream
frame" check in mpf_ops_write at random points in the transfer.
Replacing poll_status_not_busy with the above allows it to run
to completion.
On Mon, May 09, 2022 at 11:41:18AM +0000, [email protected] wrote:
> Hey Ivan, one comment below.
> Thanks,
> Conor.
>
> On 07/05/2022 08:43, Ivan Bornyakov wrote:
> > ... snip ...
> > +static int mpf_read_status(struct spi_device *spi)
> > +{
> > + u8 status, status_command = MPF_SPI_READ_STATUS;
> > + struct spi_transfer xfer = {
> > + .tx_buf = &status_command,
> > + .rx_buf = &status,
> > + .len = 1,
> > + };
> > + int ret = spi_sync_transfer(spi, &xfer, 1);
> > +
> > + if ((status & MPF_STATUS_SPI_VIOLATION) ||
> > + (status & MPF_STATUS_SPI_ERROR))
> > + ret = -EIO;
> > +
> > + return ret ? : status;
> > +}
> > +
> > ... snip ...
> > +
> > +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> > +{
> > + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> > +
> > + while (timeout--) {
> > + status = mpf_read_status(spi);
> > + if (status < 0 ||
> > + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> > + return status;
> > +
> > + usleep_range(1000, 2000);
> > + }
> > +
> > + return -EBUSY;
> > +}
>
> Is there a reason you changed this from the snippet you sent me
> in the responses to version 8:
> static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> {
> u8 status, status_command = MPF_SPI_READ_STATUS;
> int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
> struct spi_transfer xfer = {
> .tx_buf = &status_command,
> .rx_buf = &status,
> .len = 1,
> };
>
> while (timeout--) {
> ret = spi_sync_transfer(spi, &xfer, 1);
> if (ret < 0)
> return ret;
>
> if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
> return status;
>
> usleep_range(1000, 2000);
> }
>
> return -EBUSY;
> }
>
> With the current version, I hit the "Failed to write bitstream
> frame" check in mpf_ops_write at random points in the transfer.
> Replacing poll_status_not_busy with the above allows it to run
> to completion.
In my eyes they are equivalent, aren't they?
On 09/05/2022 18:16, Ivan Bornyakov wrote:
> On Mon, May 09, 2022 at 11:41:18AM +0000, [email protected] wrote:
>> Hey Ivan, one comment below.
>> Thanks,
>> Conor.
>>
>> On 07/05/2022 08:43, Ivan Bornyakov wrote:
>>> ... snip ...
>>> +static int mpf_read_status(struct spi_device *spi)
>>> +{
>>> + u8 status, status_command = MPF_SPI_READ_STATUS;
>>> + struct spi_transfer xfer = {
>>> + .tx_buf = &status_command,
>>> + .rx_buf = &status,
>>> + .len = 1,
>>> + };
>>> + int ret = spi_sync_transfer(spi, &xfer, 1);
>>> +
>>> + if ((status & MPF_STATUS_SPI_VIOLATION) ||
>>> + (status & MPF_STATUS_SPI_ERROR))
>>> + ret = -EIO;
>>> +
>>> + return ret ? : status;
>>> +}
>>> +
>>> ... snip ...
>>> +
>>> +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
>>> +{
>>> + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
>>> +
>>> + while (timeout--) {
>>> + status = mpf_read_status(spi);
>>> + if (status < 0 ||
>>> + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
>>> + return status;
>>> +
>>> + usleep_range(1000, 2000);
>>> + }
>>> +
>>> + return -EBUSY;
>>> +}
>>
>> Is there a reason you changed this from the snippet you sent me
>> in the responses to version 8:
>> static int poll_status_not_busy(struct spi_device *spi, u8 mask)
>> {
>> u8 status, status_command = MPF_SPI_READ_STATUS;
>> int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
>> struct spi_transfer xfer = {
>> .tx_buf = &status_command,
>> .rx_buf = &status,
>> .len = 1,
>> };
>>
>> while (timeout--) {
>> ret = spi_sync_transfer(spi, &xfer, 1);
>> if (ret < 0)
>> return ret;
>>
>> if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
>> return status;
>>
>> usleep_range(1000, 2000);
>> }
>>
>> return -EBUSY;
>> }
>>
>> With the current version, I hit the "Failed to write bitstream
>> frame" check in mpf_ops_write at random points in the transfer.
>> Replacing poll_status_not_busy with the above allows it to run
>> to completion.
>
> In my eyes they are equivalent, aren't they?
>
I was in a bit of a rush today & didn't have time to do proper
debugging, I'll put some debug code in tomorrow and try to find
exactly what is different between the two.
Off the top of my head, since I don't have a board on me to test,
the only difference I can see is that with the snippet you only
checked if spi_sync_transfer was negative whereas now you check
if it has a value at all w/ that ternary operator.
But even that seems like it *shouldn't* be the problem, since ret
should contain -errno or zero, right?
Either way, I will do some digging tomorrow.
Thanks,
Conor.
On 09/05/2022 19:56, Conor Dooley wrote:
> On 09/05/2022 18:16, Ivan Bornyakov wrote:
>> On Mon, May 09, 2022 at 11:41:18AM +0000, [email protected] wrote:
>>> Hey Ivan, one comment below.
>>> Thanks,
>>> Conor.
>>>
>>> On 07/05/2022 08:43, Ivan Bornyakov wrote:
>>>> ... snip ...
>>>> +static int mpf_read_status(struct spi_device *spi)
>>>> +{
>>>> + u8 status, status_command = MPF_SPI_READ_STATUS;
>>>> + struct spi_transfer xfer = {
>>>> + .tx_buf = &status_command,
>>>> + .rx_buf = &status,
>>>> + .len = 1,
>>>> + };
>>>> + int ret = spi_sync_transfer(spi, &xfer, 1);
>>>> +
>>>> + if ((status & MPF_STATUS_SPI_VIOLATION) ||
>>>> + (status & MPF_STATUS_SPI_ERROR))
>>>> + ret = -EIO;
>>>> +
>>>> + return ret ? : status;
>>>> +}
>>>> +
>>>> ... snip ...
>>>> +
>>>> +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
>>>> +{
>>>> + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
>>>> +
>>>> + while (timeout--) {
>>>> + status = mpf_read_status(spi);
>>>> + if (status < 0 ||
>>>> + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
>>>> + return status;
>>>> +
>>>> + usleep_range(1000, 2000);
>>>> + }
>>>> +
>>>> + return -EBUSY;
>>>> +}
>>>
>>> Is there a reason you changed this from the snippet you sent me
>>> in the responses to version 8:
>>> static int poll_status_not_busy(struct spi_device *spi, u8 mask)
>>> {
>>> u8 status, status_command = MPF_SPI_READ_STATUS;
>>> int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
>>> struct spi_transfer xfer = {
>>> .tx_buf = &status_command,
>>> .rx_buf = &status,
>>> .len = 1,
>>> };
>>>
>>> while (timeout--) {
>>> ret = spi_sync_transfer(spi, &xfer, 1);
>>> if (ret < 0)
>>> return ret;
>>>
>>> if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
>>> return status;
>>>
>>> usleep_range(1000, 2000);
>>> }
>>>
>>> return -EBUSY;
>>> }
>>>
>>> With the current version, I hit the "Failed to write bitstream
>>> frame" check in mpf_ops_write at random points in the transfer.
>>> Replacing poll_status_not_busy with the above allows it to run
>>> to completion.
>>
>> In my eyes they are equivalent, aren't they?
>>
>
> I was in a bit of a rush today & didn't have time to do proper
> debugging, I'll put some debug code in tomorrow and try to find
> exactly what is different between the two.
>
> Off the top of my head, since I don't have a board on me to test,
> the only difference I can see is that with the snippet you only
> checked if spi_sync_transfer was negative whereas now you check
> if it has a value at all w/ that ternary operator.
>
> But even that seems like it *shouldn't* be the problem, since ret
> should contain -errno or zero, right?
> Either way, I will do some digging tomorrow.
I put a printk("status %x, ret %d", status, ret); into the failure
path of mpf_read_status() & it looks like a status 0xA is being
returned - error & ready? That seems like a very odd combo to be
getting back out of it. It shouldn't be dodgy driver/connection
either, b/c that's what I see if I connect my protocol analyser:
https://i.imgur.com/VbjgfCk.png
That's mosi (hex), ss, sclk, mosi, miso (hex), miso in descending
order.
I think what was happening was with the snippet you returned one
of the following: -EBUSY, ret (aka -errno) or status. Since status
is positive, the checks in mpf_spi_write.*() saw nothing wrong at
all and programming continued despite there being a problem.
The new version fixes this by returning -EIO rather than status from
poll_status_not_busy().
I wish I had a socketable PolarFire so I could investigate further,
but this looks like it might a be hardware issue somewhere on my
end?
So ye, sorry for the noise and carry on! I'll try tofind what is to
blame for it.
Thanks,
Conor.
On Tue, May 10, 2022 at 12:29:54PM +0100, Conor Dooley wrote:
> On 09/05/2022 19:56, Conor Dooley wrote:
> > On 09/05/2022 18:16, Ivan Bornyakov wrote:
> > > On Mon, May 09, 2022 at 11:41:18AM +0000, [email protected] wrote:
> > > > Hey Ivan, one comment below.
> > > > Thanks,
> > > > Conor.
> > > >
> > > > On 07/05/2022 08:43, Ivan Bornyakov wrote:
> > > > > ... snip ...
> > > > > +static int mpf_read_status(struct spi_device *spi)
> > > > > +{
> > > > > + u8 status, status_command = MPF_SPI_READ_STATUS;
> > > > > + struct spi_transfer xfer = {
> > > > > + .tx_buf = &status_command,
> > > > > + .rx_buf = &status,
> > > > > + .len = 1,
> > > > > + };
> > > > > + int ret = spi_sync_transfer(spi, &xfer, 1);
> > > > > +
> > > > > + if ((status & MPF_STATUS_SPI_VIOLATION) ||
> > > > > + (status & MPF_STATUS_SPI_ERROR))
> > > > > + ret = -EIO;
> > > > > +
> > > > > + return ret ? : status;
> > > > > +}
> > > > > +
> > > > > ... snip ...
> > > > > +
> > > > > +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> > > > > +{
> > > > > + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> > > > > +
> > > > > + while (timeout--) {
> > > > > + status = mpf_read_status(spi);
> > > > > + if (status < 0 ||
> > > > > + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> > > > > + return status;
> > > > > +
> > > > > + usleep_range(1000, 2000);
> > > > > + }
> > > > > +
> > > > > + return -EBUSY;
> > > > > +}
> > > >
> > > > Is there a reason you changed this from the snippet you sent me
> > > > in the responses to version 8:
> > > > static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> > > > {
> > > > u8 status, status_command = MPF_SPI_READ_STATUS;
> > > > int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
> > > > struct spi_transfer xfer = {
> > > > .tx_buf = &status_command,
> > > > .rx_buf = &status,
> > > > .len = 1,
> > > > };
> > > >
> > > > while (timeout--) {
> > > > ret = spi_sync_transfer(spi, &xfer, 1);
> > > > if (ret < 0)
> > > > return ret;
> > > >
> > > > if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
> > > > return status;
> > > >
> > > > usleep_range(1000, 2000);
> > > > }
> > > >
> > > > return -EBUSY;
> > > > }
> > > >
> > > > With the current version, I hit the "Failed to write bitstream
> > > > frame" check in mpf_ops_write at random points in the transfer.
> > > > Replacing poll_status_not_busy with the above allows it to run
> > > > to completion.
> > >
> > > In my eyes they are equivalent, aren't they?
> > >
> >
> > I was in a bit of a rush today & didn't have time to do proper
> > debugging, I'll put some debug code in tomorrow and try to find
> > exactly what is different between the two.
> >
> > Off the top of my head, since I don't have a board on me to test,
> > the only difference I can see is that with the snippet you only
> > checked if spi_sync_transfer was negative whereas now you check
> > if it has a value at all w/ that ternary operator.
> >
> > But even that seems like it *shouldn't* be the problem, since ret
> > should contain -errno or zero, right?
> > Either way, I will do some digging tomorrow.
>
> I put a printk("status %x, ret %d", status, ret); into the failure
> path of mpf_read_status() & it looks like a status 0xA is being
> returned - error & ready? That seems like a very odd combo to be
> getting back out of it. It shouldn't be dodgy driver/connection
> either, b/c that's what I see if I connect my protocol analyser:
> https://i.imgur.com/VbjgfCk.png
>
> That's mosi (hex), ss, sclk, mosi, miso (hex), miso in descending
> order.
>
> I think what was happening was with the snippet you returned one
> of the following: -EBUSY, ret (aka -errno) or status. Since status
> is positive, the checks in mpf_spi_write.*() saw nothing wrong at
> all and programming continued despite there being a problem.
>
> The new version fixes this by returning -EIO rather than status from
> poll_status_not_busy().
>
> I wish I had a socketable PolarFire so I could investigate further,
> but this looks like it might a be hardware issue somewhere on my
> end?
>
> So ye, sorry for the noise and carry on! I'll try tofind what is to
> blame for it.
>
> Thanks,
> Conor.
>
Hi, Conor.
I've just noticed in SPI-DirectC User Guide [1] ch. 9 SmartFusion2 and
IGLOO2 SPI-Slave Programming Waveform Analysis, that hw status checked
two times every time. Does MPF family also need double check hw status?
Does adding second mpf_read_status() to poll_status_not_busy() routine
help with your issue?
On Wed, May 11, 2022 at 11:15:32AM +0300, Ivan Bornyakov wrote:
> On Tue, May 10, 2022 at 12:29:54PM +0100, Conor Dooley wrote:
> > On 09/05/2022 19:56, Conor Dooley wrote:
> > > On 09/05/2022 18:16, Ivan Bornyakov wrote:
> > > > On Mon, May 09, 2022 at 11:41:18AM +0000, [email protected] wrote:
> > > > > Hey Ivan, one comment below.
> > > > > Thanks,
> > > > > Conor.
> > > > >
> > > > > On 07/05/2022 08:43, Ivan Bornyakov wrote:
> > > > > > ... snip ...
> > > > > > +static int mpf_read_status(struct spi_device *spi)
> > > > > > +{
> > > > > > + u8 status, status_command = MPF_SPI_READ_STATUS;
> > > > > > + struct spi_transfer xfer = {
> > > > > > + .tx_buf = &status_command,
> > > > > > + .rx_buf = &status,
> > > > > > + .len = 1,
> > > > > > + };
> > > > > > + int ret = spi_sync_transfer(spi, &xfer, 1);
> > > > > > +
> > > > > > + if ((status & MPF_STATUS_SPI_VIOLATION) ||
> > > > > > + (status & MPF_STATUS_SPI_ERROR))
> > > > > > + ret = -EIO;
> > > > > > +
> > > > > > + return ret ? : status;
> > > > > > +}
> > > > > > +
> > > > > > ... snip ...
> > > > > > +
> > > > > > +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> > > > > > +{
> > > > > > + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> > > > > > +
> > > > > > + while (timeout--) {
> > > > > > + status = mpf_read_status(spi);
> > > > > > + if (status < 0 ||
> > > > > > + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> > > > > > + return status;
> > > > > > +
> > > > > > + usleep_range(1000, 2000);
> > > > > > + }
> > > > > > +
> > > > > > + return -EBUSY;
> > > > > > +}
> > > > >
> > > > > Is there a reason you changed this from the snippet you sent me
> > > > > in the responses to version 8:
> > > > > static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> > > > > {
> > > > > u8 status, status_command = MPF_SPI_READ_STATUS;
> > > > > int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
> > > > > struct spi_transfer xfer = {
> > > > > .tx_buf = &status_command,
> > > > > .rx_buf = &status,
> > > > > .len = 1,
> > > > > };
> > > > >
> > > > > while (timeout--) {
> > > > > ret = spi_sync_transfer(spi, &xfer, 1);
> > > > > if (ret < 0)
> > > > > return ret;
> > > > >
> > > > > if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
> > > > > return status;
> > > > >
> > > > > usleep_range(1000, 2000);
> > > > > }
> > > > >
> > > > > return -EBUSY;
> > > > > }
> > > > >
> > > > > With the current version, I hit the "Failed to write bitstream
> > > > > frame" check in mpf_ops_write at random points in the transfer.
> > > > > Replacing poll_status_not_busy with the above allows it to run
> > > > > to completion.
> > > >
> > > > In my eyes they are equivalent, aren't they?
> > > >
> > >
> > > I was in a bit of a rush today & didn't have time to do proper
> > > debugging, I'll put some debug code in tomorrow and try to find
> > > exactly what is different between the two.
> > >
> > > Off the top of my head, since I don't have a board on me to test,
> > > the only difference I can see is that with the snippet you only
> > > checked if spi_sync_transfer was negative whereas now you check
> > > if it has a value at all w/ that ternary operator.
> > >
> > > But even that seems like it *shouldn't* be the problem, since ret
> > > should contain -errno or zero, right?
> > > Either way, I will do some digging tomorrow.
> >
> > I put a printk("status %x, ret %d", status, ret); into the failure
> > path of mpf_read_status() & it looks like a status 0xA is being
> > returned - error & ready? That seems like a very odd combo to be
> > getting back out of it. It shouldn't be dodgy driver/connection
> > either, b/c that's what I see if I connect my protocol analyser:
> > https://i.imgur.com/VbjgfCk.png
> >
> > That's mosi (hex), ss, sclk, mosi, miso (hex), miso in descending
> > order.
> >
> > I think what was happening was with the snippet you returned one
> > of the following: -EBUSY, ret (aka -errno) or status. Since status
> > is positive, the checks in mpf_spi_write.*() saw nothing wrong at
> > all and programming continued despite there being a problem.
> >
> > The new version fixes this by returning -EIO rather than status from
> > poll_status_not_busy().
> >
> > I wish I had a socketable PolarFire so I could investigate further,
> > but this looks like it might a be hardware issue somewhere on my
> > end?
> >
> > So ye, sorry for the noise and carry on! I'll try tofind what is to
> > blame for it.
> >
> > Thanks,
> > Conor.
> >
>
> Hi, Conor.
>
> I've just noticed in SPI-DirectC User Guide [1] ch. 9 SmartFusion2 and
> IGLOO2 SPI-Slave Programming Waveform Analysis, that hw status checked
> two times every time. Does MPF family also need double check hw status?
> Does adding second mpf_read_status() to poll_status_not_busy() routine
> help with your issue?
Sorry, forgot to insert link to SPI-DirectC User Guide. Here it is:
[1] https://coredocs.s3.amazonaws.com/DirectC/2021_2/spi_directc.pdf
On 11/05/2022 09:15, Ivan Bornyakov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, May 10, 2022 at 12:29:54PM +0100, Conor Dooley wrote:
>> On 09/05/2022 19:56, Conor Dooley wrote:
>>> On 09/05/2022 18:16, Ivan Bornyakov wrote:
>>>> On Mon, May 09, 2022 at 11:41:18AM +0000, [email protected] wrote:
>>>>> Hey Ivan, one comment below.
>>>>> Thanks,
>>>>> Conor.
>>>>>
>>>>> On 07/05/2022 08:43, Ivan Bornyakov wrote:
>>>>>> ... snip ...
>>>>>> +static int mpf_read_status(struct spi_device *spi)
>>>>>> +{
>>>>>> + u8 status, status_command = MPF_SPI_READ_STATUS;
>>>>>> + struct spi_transfer xfer = {
>>>>>> + .tx_buf = &status_command,
>>>>>> + .rx_buf = &status,
>>>>>> + .len = 1,
>>>>>> + };
>>>>>> + int ret = spi_sync_transfer(spi, &xfer, 1);
>>>>>> +
>>>>>> + if ((status & MPF_STATUS_SPI_VIOLATION) ||
>>>>>> + (status & MPF_STATUS_SPI_ERROR))
>>>>>> + ret = -EIO;
>>>>>> +
>>>>>> + return ret ? : status;
>>>>>> +}
>>>>>> +
>>>>>> ... snip ...
>>>>>> +
>>>>>> +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
>>>>>> +{
>>>>>> + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
>>>>>> +
>>>>>> + while (timeout--) {
>>>>>> + status = mpf_read_status(spi);
>>>>>> + if (status < 0 ||
>>>>>> + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
>>>>>> + return status;
>>>>>> +
>>>>>> + usleep_range(1000, 2000);
>>>>>> + }
>>>>>> +
>>>>>> + return -EBUSY;
>>>>>> +}
>>>>>
>>>>> Is there a reason you changed this from the snippet you sent me
>>>>> in the responses to version 8:
>>>>> static int poll_status_not_busy(struct spi_device *spi, u8 mask)
>>>>> {
>>>>> u8 status, status_command = MPF_SPI_READ_STATUS;
>>>>> int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
>>>>> struct spi_transfer xfer = {
>>>>> .tx_buf = &status_command,
>>>>> .rx_buf = &status,
>>>>> .len = 1,
>>>>> };
>>>>>
>>>>> while (timeout--) {
>>>>> ret = spi_sync_transfer(spi, &xfer, 1);
>>>>> if (ret < 0)
>>>>> return ret;
>>>>>
>>>>> if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
>>>>> return status;
>>>>>
>>>>> usleep_range(1000, 2000);
>>>>> }
>>>>>
>>>>> return -EBUSY;
>>>>> }
>>>>>
>>>>> With the current version, I hit the "Failed to write bitstream
>>>>> frame" check in mpf_ops_write at random points in the transfer.
>>>>> Replacing poll_status_not_busy with the above allows it to run
>>>>> to completion.
>>>>
>>>> In my eyes they are equivalent, aren't they?
>>>>
>>>
>>> I was in a bit of a rush today & didn't have time to do proper
>>> debugging, I'll put some debug code in tomorrow and try to find
>>> exactly what is different between the two.
>>>
>>> Off the top of my head, since I don't have a board on me to test,
>>> the only difference I can see is that with the snippet you only
>>> checked if spi_sync_transfer was negative whereas now you check
>>> if it has a value at all w/ that ternary operator.
>>>
>>> But even that seems like it *shouldn't* be the problem, since ret
>>> should contain -errno or zero, right?
>>> Either way, I will do some digging tomorrow.
>>
>> I put a printk("status %x, ret %d", status, ret); into the failure
>> path of mpf_read_status() & it looks like a status 0xA is being
>> returned - error & ready? That seems like a very odd combo to be
>> getting back out of it. It shouldn't be dodgy driver/connection
>> either, b/c that's what I see if I connect my protocol analyser:
>> https://i.imgur.com/VbjgfCk.png
>>
>> That's mosi (hex), ss, sclk, mosi, miso (hex), miso in descending
>> order.
>>
>> I think what was happening was with the snippet you returned one
>> of the following: -EBUSY, ret (aka -errno) or status. Since status
>> is positive, the checks in mpf_spi_write.*() saw nothing wrong at
>> all and programming continued despite there being a problem.
>>
>> The new version fixes this by returning -EIO rather than status from
>> poll_status_not_busy().
>>
>> I wish I had a socketable PolarFire so I could investigate further,
>> but this looks like it might a be hardware issue somewhere on my
>> end?
>>
>> So ye, sorry for the noise and carry on! I'll try tofind what is to
>> blame for it.
>>
>> Thanks,
>> Conor.
>>
>
> Hi, Conor.
>
> I've just noticed in SPI-DirectC User Guide [1] ch. 9 SmartFusion2 and
> IGLOO2 SPI-Slave Programming Waveform Analysis, that hw status checked
> two times every time. Does MPF family also need double check hw status?
> Does adding second mpf_read_status() to poll_status_not_busy() routine
> help with your issue?
Hey Ivan,
Tried your suggestion. Previously I was failing quite consistently at
transfer 34 of 590k, and sometimes making it a further. With your
suggestion, I was making it significantly further (100k+) but still
running into some of the 0xA status.
Decided to move the double check into mpfs_read_status (see the below
diff) did not run into any the 0xA statuses.
It's worth pointing out that this is the *first* time I have seen
Flash Pro Express report that the FPGA array has been enabled after
programming!
Seems like at the very least this (hacky) diff is not harmful?
Please give it a try yourself and check that things still work for
you.
diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
index 63b75dff2522..183cdfc05c4a 100644
--- a/drivers/fpga/microchip-spi.c
+++ b/drivers/fpga/microchip-spi.c
@@ -47,18 +47,30 @@ struct mpf_priv {
static int mpf_read_status(struct spi_device *spi)
{
u8 status, status_command = MPF_SPI_READ_STATUS;
+ u8 status_repeat;
struct spi_transfer xfer = {
.tx_buf = &status_command,
.rx_buf = &status,
.len = 1,
};
+ struct spi_transfer xfer_repeat = {
+ .tx_buf = &status_command,
+ .rx_buf = &status_repeat,
+ .len = 1,
+ };
int ret = spi_sync_transfer(spi, &xfer, 1);
+ int ret_repeat = spi_sync_transfer(spi, &xfer_repeat, 1);
+
+ if (ret || ret_repeat)
+ return -EIO;
- if ((status & MPF_STATUS_SPI_VIOLATION) ||
- (status & MPF_STATUS_SPI_ERROR))
+ if (status != status_repeat)
+ printk("status disagreement %x %x", status, status_repeat);
+ if ((status_repeat & MPF_STATUS_SPI_VIOLATION) ||
+ (status_repeat & MPF_STATUS_SPI_ERROR))
ret = -EIO;
- return ret ? : status;
+ return ret ?: status_repeat;
}
static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr)
On Wed, May 11, 2022 at 11:36:33AM +0000, [email protected] wrote:
> On 11/05/2022 09:15, Ivan Bornyakov wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Tue, May 10, 2022 at 12:29:54PM +0100, Conor Dooley wrote:
> >> On 09/05/2022 19:56, Conor Dooley wrote:
> >>> On 09/05/2022 18:16, Ivan Bornyakov wrote:
> >>>> On Mon, May 09, 2022 at 11:41:18AM +0000, [email protected] wrote:
> >>>>> Hey Ivan, one comment below.
> >>>>> Thanks,
> >>>>> Conor.
> >>>>>
> >>>>> On 07/05/2022 08:43, Ivan Bornyakov wrote:
> >>>>>> ... snip ...
> >>>>>> +static int mpf_read_status(struct spi_device *spi)
> >>>>>> +{
> >>>>>> + u8 status, status_command = MPF_SPI_READ_STATUS;
> >>>>>> + struct spi_transfer xfer = {
> >>>>>> + .tx_buf = &status_command,
> >>>>>> + .rx_buf = &status,
> >>>>>> + .len = 1,
> >>>>>> + };
> >>>>>> + int ret = spi_sync_transfer(spi, &xfer, 1);
> >>>>>> +
> >>>>>> + if ((status & MPF_STATUS_SPI_VIOLATION) ||
> >>>>>> + (status & MPF_STATUS_SPI_ERROR))
> >>>>>> + ret = -EIO;
> >>>>>> +
> >>>>>> + return ret ? : status;
> >>>>>> +}
> >>>>>> +
> >>>>>> ... snip ...
> >>>>>> +
> >>>>>> +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> >>>>>> +{
> >>>>>> + int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> >>>>>> +
> >>>>>> + while (timeout--) {
> >>>>>> + status = mpf_read_status(spi);
> >>>>>> + if (status < 0 ||
> >>>>>> + (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> >>>>>> + return status;
> >>>>>> +
> >>>>>> + usleep_range(1000, 2000);
> >>>>>> + }
> >>>>>> +
> >>>>>> + return -EBUSY;
> >>>>>> +}
> >>>>>
> >>>>> Is there a reason you changed this from the snippet you sent me
> >>>>> in the responses to version 8:
> >>>>> static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> >>>>> {
> >>>>> u8 status, status_command = MPF_SPI_READ_STATUS;
> >>>>> int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
> >>>>> struct spi_transfer xfer = {
> >>>>> .tx_buf = &status_command,
> >>>>> .rx_buf = &status,
> >>>>> .len = 1,
> >>>>> };
> >>>>>
> >>>>> while (timeout--) {
> >>>>> ret = spi_sync_transfer(spi, &xfer, 1);
> >>>>> if (ret < 0)
> >>>>> return ret;
> >>>>>
> >>>>> if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
> >>>>> return status;
> >>>>>
> >>>>> usleep_range(1000, 2000);
> >>>>> }
> >>>>>
> >>>>> return -EBUSY;
> >>>>> }
> >>>>>
> >>>>> With the current version, I hit the "Failed to write bitstream
> >>>>> frame" check in mpf_ops_write at random points in the transfer.
> >>>>> Replacing poll_status_not_busy with the above allows it to run
> >>>>> to completion.
> >>>>
> >>>> In my eyes they are equivalent, aren't they?
> >>>>
> >>>
> >>> I was in a bit of a rush today & didn't have time to do proper
> >>> debugging, I'll put some debug code in tomorrow and try to find
> >>> exactly what is different between the two.
> >>>
> >>> Off the top of my head, since I don't have a board on me to test,
> >>> the only difference I can see is that with the snippet you only
> >>> checked if spi_sync_transfer was negative whereas now you check
> >>> if it has a value at all w/ that ternary operator.
> >>>
> >>> But even that seems like it *shouldn't* be the problem, since ret
> >>> should contain -errno or zero, right?
> >>> Either way, I will do some digging tomorrow.
> >>
> >> I put a printk("status %x, ret %d", status, ret); into the failure
> >> path of mpf_read_status() & it looks like a status 0xA is being
> >> returned - error & ready? That seems like a very odd combo to be
> >> getting back out of it. It shouldn't be dodgy driver/connection
> >> either, b/c that's what I see if I connect my protocol analyser:
> >> https://i.imgur.com/VbjgfCk.png
> >>
> >> That's mosi (hex), ss, sclk, mosi, miso (hex), miso in descending
> >> order.
> >>
> >> I think what was happening was with the snippet you returned one
> >> of the following: -EBUSY, ret (aka -errno) or status. Since status
> >> is positive, the checks in mpf_spi_write.*() saw nothing wrong at
> >> all and programming continued despite there being a problem.
> >>
> >> The new version fixes this by returning -EIO rather than status from
> >> poll_status_not_busy().
> >>
> >> I wish I had a socketable PolarFire so I could investigate further,
> >> but this looks like it might a be hardware issue somewhere on my
> >> end?
> >>
> >> So ye, sorry for the noise and carry on! I'll try tofind what is to
> >> blame for it.
> >>
> >> Thanks,
> >> Conor.
> >>
> >
> > Hi, Conor.
> >
> > I've just noticed in SPI-DirectC User Guide [1] ch. 9 SmartFusion2 and
> > IGLOO2 SPI-Slave Programming Waveform Analysis, that hw status checked
> > two times every time. Does MPF family also need double check hw status?
> > Does adding second mpf_read_status() to poll_status_not_busy() routine
> > help with your issue?
>
> Hey Ivan,
> Tried your suggestion. Previously I was failing quite consistently at
> transfer 34 of 590k, and sometimes making it a further. With your
> suggestion, I was making it significantly further (100k+) but still
> running into some of the 0xA status.
> Decided to move the double check into mpfs_read_status (see the below
> diff) did not run into any the 0xA statuses.
> It's worth pointing out that this is the *first* time I have seen
> Flash Pro Express report that the FPGA array has been enabled after
> programming!
That's good news!
> Seems like at the very least this (hacky) diff is not harmful?
> Please give it a try yourself and check that things still work for
> you.
>
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> index 63b75dff2522..183cdfc05c4a 100644
> --- a/drivers/fpga/microchip-spi.c
> +++ b/drivers/fpga/microchip-spi.c
> @@ -47,18 +47,30 @@ struct mpf_priv {
> static int mpf_read_status(struct spi_device *spi)
> {
> u8 status, status_command = MPF_SPI_READ_STATUS;
> + u8 status_repeat;
> struct spi_transfer xfer = {
> .tx_buf = &status_command,
> .rx_buf = &status,
> .len = 1,
> };
> + struct spi_transfer xfer_repeat = {
> + .tx_buf = &status_command,
> + .rx_buf = &status_repeat,
> + .len = 1,
> + };
> int ret = spi_sync_transfer(spi, &xfer, 1);
> + int ret_repeat = spi_sync_transfer(spi, &xfer_repeat, 1);
> +
> + if (ret || ret_repeat)
> + return -EIO;
>
> - if ((status & MPF_STATUS_SPI_VIOLATION) ||
> - (status & MPF_STATUS_SPI_ERROR))
> + if (status != status_repeat)
> + printk("status disagreement %x %x", status, status_repeat);
> + if ((status_repeat & MPF_STATUS_SPI_VIOLATION) ||
> + (status_repeat & MPF_STATUS_SPI_ERROR))
> ret = -EIO;
>
> - return ret ? : status;
> + return ret ?: status_repeat;
> }
>
> static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr)
I'll check that and send out v12 if it's all rigth in the near future.
