[PATCH v2 0/3] Reliability improvements for Microchip MPF FPGA manager

[PATCH v2 0/3] Reliability improvements for Microchip MPF FPGA manager

[Ivan Bornyakov]

A couple of reliability improvements for Microchip Polarfire FPGA
manager:
 * move SPI I/O buffers out of stack
 * rewrite status polling routine in a time measurable way

Also improve mpf_ops_write() code readability by separating single data
frame writing routine.

ChangeLog:
  v1:
[https://lore.kernel.org/linux-fpga/20221223123854.8023-1-i.bornyakov@metrotek.ru/]
  v2:
    * split into 3 distinct patches

Ivan Bornyakov (3):
  fpga: microchip-spi: move SPI I/O buffers out of stack
  fpga: microchip-spi: rewrite status polling in a time measurable way
  fpga: microchip-spi: separate data frame write routine

 drivers/fpga/microchip-spi.c | 138 ++++++++++++++++++-----------------
 1 file changed, 73 insertions(+), 65 deletions(-)

-- 
2.38.2

[PATCH v2 1/3] fpga: microchip-spi: move SPI I/O buffers out of stack

[Ivan Bornyakov]

As spi-summary doc says:
 > I/O buffers use the usual Linux rules, and must be DMA-safe.
 > You'd normally allocate them from the heap or free page pool.
 > Don't use the stack, or anything that's declared "static".

Replace spi_write() with spi_write_then_read(), which is dma-safe for
on-stack buffers. Use allocated buffers for transfers used in
spi_sync_transfer().

Although everything works OK with stack-located I/O buffers, better
follow the doc to be safe.

Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
---
 drivers/fpga/microchip-spi.c | 93 ++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
index 7436976ea904..e72fedd93a27 100644
--- a/drivers/fpga/microchip-spi.c
+++ b/drivers/fpga/microchip-spi.c
@@ -42,46 +42,55 @@
 struct mpf_priv {
 	struct spi_device *spi;
 	bool program_mode;
+	u8 tx __aligned(ARCH_KMALLOC_MINALIGN);
+	u8 rx __aligned(ARCH_KMALLOC_MINALIGN);
 };
 
-static int mpf_read_status(struct spi_device *spi)
+static int mpf_read_status(struct mpf_priv *priv)
 {
-	u8 status = 0, status_command = MPF_SPI_READ_STATUS;
-	struct spi_transfer xfers[2] = { 0 };
-	int ret;
-
 	/*
 	 * HW status is returned on MISO in the first byte after CS went
 	 * active. However, first reading can be inadequate, so we submit
 	 * two identical SPI transfers and use result of the later one.
 	 */
-	xfers[0].tx_buf = &status_command;
-	xfers[1].tx_buf = &status_command;
-	xfers[0].rx_buf = &status;
-	xfers[1].rx_buf = &status;
-	xfers[0].len = 1;
-	xfers[1].len = 1;
-	xfers[0].cs_change = 1;
+	struct spi_transfer xfers[2] = {
+		{
+			.tx_buf = &priv->tx,
+			.rx_buf = &priv->rx,
+			.len = 1,
+			.cs_change = 1,
+		}, {
+			.tx_buf = &priv->tx,
+			.rx_buf = &priv->rx,
+			.len = 1,
+		},
+	};
+	u8 status;
+	int ret;
+
+	priv->tx = MPF_SPI_READ_STATUS;
+
+	ret = spi_sync_transfer(priv->spi, xfers, 2);
+	if (ret)
+		return ret;
 
-	ret = spi_sync_transfer(spi, xfers, 2);
+	status = priv->rx;
 
 	if ((status & MPF_STATUS_SPI_VIOLATION) ||
 	    (status & MPF_STATUS_SPI_ERROR))
-		ret = -EIO;
+		return -EIO;
 
-	return ret ? : status;
+	return 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);
+	status = mpf_read_status(priv);
 
 	if (!program_mode && !status)
 		return FPGA_MGR_STATE_OPERATING;
@@ -186,12 +195,12 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
 }
 
 /* Poll HW status until busy bit is cleared and mask bits are set. */
-static int mpf_poll_status(struct spi_device *spi, u8 mask)
+static int mpf_poll_status(struct mpf_priv *priv, u8 mask)
 {
 	int status, retries = MPF_STATUS_POLL_RETRIES;
 
 	while (retries--) {
-		status = mpf_read_status(spi);
+		status = mpf_read_status(priv);
 		if (status < 0)
 			return status;
 
@@ -205,32 +214,32 @@ static int mpf_poll_status(struct spi_device *spi, u8 mask)
 	return -EBUSY;
 }
 
-static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size)
+static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t buf_size)
 {
-	int status = mpf_poll_status(spi, 0);
+	int status = mpf_poll_status(priv, 0);
 
 	if (status < 0)
 		return status;
 
-	return spi_write(spi, buf, buf_size);
+	return spi_write_then_read(priv->spi, buf, buf_size, NULL, 0);
 }
 
-static int mpf_spi_write_then_read(struct spi_device *spi,
+static int mpf_spi_write_then_read(struct mpf_priv *priv,
 				   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);
+	ret = mpf_spi_write(priv, txbuf, txbuf_size);
 	if (ret)
 		return ret;
 
-	ret = mpf_poll_status(spi, MPF_STATUS_READY);
+	ret = mpf_poll_status(priv, MPF_STATUS_READY);
 	if (ret < 0)
 		return ret;
 
-	return spi_write_then_read(spi, read_command, sizeof(read_command),
+	return spi_write_then_read(priv->spi, read_command, sizeof(read_command),
 				   rxbuf, rxbuf_size);
 }
 
@@ -242,7 +251,6 @@ static int mpf_ops_write_init(struct fpga_manager *mgr,
 	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 = 0;
 	int ret;
 
@@ -251,9 +259,7 @@ static int mpf_ops_write_init(struct fpga_manager *mgr,
 		return -EOPNOTSUPP;
 	}
 
-	spi = priv->spi;
-
-	ret = mpf_spi_write_then_read(spi, isc_en_command, sizeof(isc_en_command),
+	ret = mpf_spi_write_then_read(priv, isc_en_command, sizeof(isc_en_command),
 				      &isc_ret, sizeof(isc_ret));
 	if (ret || isc_ret) {
 		dev_err(dev, "Failed to enable ISC: spi_ret %d, isc_ret %u\n",
@@ -261,7 +267,7 @@ static int mpf_ops_write_init(struct fpga_manager *mgr,
 		return -EFAULT;
 	}
 
-	ret = mpf_spi_write(spi, program_mode, sizeof(program_mode));
+	ret = mpf_spi_write(priv, program_mode, sizeof(program_mode));
 	if (ret) {
 		dev_err(dev, "Failed to enter program mode: %d\n", ret);
 		return ret;
@@ -274,11 +280,9 @@ static int mpf_ops_write_init(struct fpga_manager *mgr,
 
 static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
 {
-	u8 spi_frame_command[] = { MPF_SPI_FRAME };
 	struct spi_transfer xfers[2] = { 0 };
 	struct mpf_priv *priv = mgr->priv;
 	struct device *dev = &mgr->dev;
-	struct spi_device *spi;
 	int ret, i;
 
 	if (count % MPF_SPI_FRAME_SIZE) {
@@ -287,18 +291,18 @@ static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count
 		return -EINVAL;
 	}
 
-	spi = priv->spi;
-
-	xfers[0].tx_buf = spi_frame_command;
-	xfers[0].len = sizeof(spi_frame_command);
+	xfers[0].tx_buf = &priv->tx;
+	xfers[0].len = 1;
 
 	for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
 		xfers[1].tx_buf = buf + i * MPF_SPI_FRAME_SIZE;
 		xfers[1].len = MPF_SPI_FRAME_SIZE;
 
-		ret = mpf_poll_status(spi, 0);
-		if (ret >= 0)
-			ret = spi_sync_transfer(spi, xfers, ARRAY_SIZE(xfers));
+		ret = mpf_poll_status(priv, 0);
+		if (ret >= 0) {
+			priv->tx = MPF_SPI_FRAME;
+			ret = spi_sync_transfer(priv->spi, xfers, ARRAY_SIZE(xfers));
+		}
 
 		if (ret) {
 			dev_err(dev, "Failed to write bitstream frame %d/%zu\n",
@@ -317,12 +321,9 @@ static int mpf_ops_write_complete(struct fpga_manager *mgr,
 	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));
+	ret = mpf_spi_write(priv, isc_dis_command, sizeof(isc_dis_command));
 	if (ret) {
 		dev_err(dev, "Failed to disable ISC: %d\n", ret);
 		return ret;
@@ -330,7 +331,7 @@ static int mpf_ops_write_complete(struct fpga_manager *mgr,
 
 	usleep_range(1000, 2000);
 
-	ret = mpf_spi_write(spi, release_command, sizeof(release_command));
+	ret = mpf_spi_write(priv, release_command, sizeof(release_command));
 	if (ret) {
 		dev_err(dev, "Failed to exit program mode: %d\n", ret);
 		return ret;
-- 
2.38.2

[PATCH v2 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way

[Ivan Bornyakov]

Original busy loop with retries count in mpf_poll_status() is not too
reliable, as it takes different times on different systems. Replace it
with read_poll_timeout() macro.

Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
---
 drivers/fpga/microchip-spi.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
index e72fedd93a27..f3ddfab87499 100644
--- a/drivers/fpga/microchip-spi.c
+++ b/drivers/fpga/microchip-spi.c
@@ -6,6 +6,7 @@
 #include <asm/unaligned.h>
 #include <linux/delay.h>
 #include <linux/fpga/fpga-mgr.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/spi/spi.h>
@@ -33,7 +34,7 @@
 
 #define	MPF_BITS_PER_COMPONENT_SIZE	22
 
-#define	MPF_STATUS_POLL_RETRIES		10000
+#define	MPF_STATUS_POLL_TIMEOUT		(2 * USEC_PER_SEC)
 #define	MPF_STATUS_BUSY			BIT(0)
 #define	MPF_STATUS_READY		BIT(1)
 #define	MPF_STATUS_SPI_VIOLATION	BIT(2)
@@ -197,21 +198,17 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
 /* Poll HW status until busy bit is cleared and mask bits are set. */
 static int mpf_poll_status(struct mpf_priv *priv, u8 mask)
 {
-	int status, retries = MPF_STATUS_POLL_RETRIES;
+	int ret, status;
 
-	while (retries--) {
-		status = mpf_read_status(priv);
-		if (status < 0)
-			return status;
-
-		if (status & MPF_STATUS_BUSY)
-			continue;
-
-		if (!mask || (status & mask))
-			return status;
-	}
+	ret = read_poll_timeout(mpf_read_status, status,
+				(status < 0) ||
+				(!(status & MPF_STATUS_BUSY) &&
+				 (!mask || (status & mask))),
+				0, MPF_STATUS_POLL_TIMEOUT, false, priv);
+	if (ret < 0)
+		return ret;
 
-	return -EBUSY;
+	return status;
 }
 
 static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t buf_size)
-- 
2.38.2

[PATCH v2 3/3] fpga: microchip-spi: separate data frame write routine

[Ivan Bornyakov]

mpf_ops_write() function writes bitstream data to the FPGA by a smaller
frames. Introduce mpf_spi_frame_write() function which is for writing a
single data frame and use it in mpf_ops_write().

No functional changes intended.

Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
---
 drivers/fpga/microchip-spi.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
index f3ddfab87499..f8a6872c865b 100644
--- a/drivers/fpga/microchip-spi.c
+++ b/drivers/fpga/microchip-spi.c
@@ -275,9 +275,30 @@ static int mpf_ops_write_init(struct fpga_manager *mgr,
 	return 0;
 }
 
+static int mpf_spi_frame_write(struct mpf_priv *priv, const char *buf)
+{
+	struct spi_transfer xfers[2] = {
+		{
+			.tx_buf = &priv->tx,
+			.len = 1,
+		}, {
+			.tx_buf = buf,
+			.len = MPF_SPI_FRAME_SIZE,
+		},
+	};
+	int ret;
+
+	ret = mpf_poll_status(priv, 0);
+	if (ret < 0)
+		return ret;
+
+	priv->tx = MPF_SPI_FRAME;
+
+	return spi_sync_transfer(priv->spi, xfers, ARRAY_SIZE(xfers));
+}
+
 static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
 {
-	struct spi_transfer xfers[2] = { 0 };
 	struct mpf_priv *priv = mgr->priv;
 	struct device *dev = &mgr->dev;
 	int ret, i;
@@ -288,19 +309,8 @@ static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count
 		return -EINVAL;
 	}
 
-	xfers[0].tx_buf = &priv->tx;
-	xfers[0].len = 1;
-
 	for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
-		xfers[1].tx_buf = buf + i * MPF_SPI_FRAME_SIZE;
-		xfers[1].len = MPF_SPI_FRAME_SIZE;
-
-		ret = mpf_poll_status(priv, 0);
-		if (ret >= 0) {
-			priv->tx = MPF_SPI_FRAME;
-			ret = spi_sync_transfer(priv->spi, xfers, ARRAY_SIZE(xfers));
-		}
-
+		ret = mpf_spi_frame_write(priv, buf + i * MPF_SPI_FRAME_SIZE);
 		if (ret) {
 			dev_err(dev, "Failed to write bitstream frame %d/%zu\n",
 				i, count / MPF_SPI_FRAME_SIZE);
-- 
2.38.2

Re: [PATCH v2 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way

[Ilpo Järvinen]

On Mon, 26 Dec 2022, Ivan Bornyakov wrote:

> Original busy loop with retries count in mpf_poll_status() is not too
> reliable, as it takes different times on different systems. Replace it
> with read_poll_timeout() macro.
> 
> Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
> Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> ---
>  drivers/fpga/microchip-spi.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> index e72fedd93a27..f3ddfab87499 100644
> --- a/drivers/fpga/microchip-spi.c
> +++ b/drivers/fpga/microchip-spi.c
> @@ -6,6 +6,7 @@
>  #include <asm/unaligned.h>
>  #include <linux/delay.h>
>  #include <linux/fpga/fpga-mgr.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/spi/spi.h>
> @@ -33,7 +34,7 @@
>  
>  #define	MPF_BITS_PER_COMPONENT_SIZE	22
>  
> -#define	MPF_STATUS_POLL_RETRIES		10000
> +#define	MPF_STATUS_POLL_TIMEOUT		(2 * USEC_PER_SEC)
>  #define	MPF_STATUS_BUSY			BIT(0)
>  #define	MPF_STATUS_READY		BIT(1)
>  #define	MPF_STATUS_SPI_VIOLATION	BIT(2)
> @@ -197,21 +198,17 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
>  /* Poll HW status until busy bit is cleared and mask bits are set. */
>  static int mpf_poll_status(struct mpf_priv *priv, u8 mask)
>  {
> -	int status, retries = MPF_STATUS_POLL_RETRIES;
> +	int ret, status;
>  
> -	while (retries--) {
> -		status = mpf_read_status(priv);
> -		if (status < 0)
> -			return status;
> -
> -		if (status & MPF_STATUS_BUSY)
> -			continue;
> -
> -		if (!mask || (status & mask))
> -			return status;
> -	}
> +	ret = read_poll_timeout(mpf_read_status, status,
> +				(status < 0) ||
> +				(!(status & MPF_STATUS_BUSY) &&
> +				 (!mask || (status & mask))),

I'm just noting that this code does not match function's comment 
(neither pre nor post diff code). The comment claims "mask bits are set" 
but the code tests "any mask bit set". 

However, currently it causes no issue because only MPF_STATUS_READY is 
passed and it's BIT(1).

I think the condition matching to the comment would be this (IMHO, it is 
also slightly simpler because !mask doesn't need to be special cased):

((status & (MPF_STATUS_BUSY | mask)) == mask)

(But not necessarily to be changed in this patch.)

-- 
 i.

> +				0, MPF_STATUS_POLL_TIMEOUT, false, priv);
> +	if (ret < 0)
> +		return ret;
>  
> -	return -EBUSY;
> +	return status;
>  }
>  
>  static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t buf_size)
> 

Re: [PATCH v2 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way

[Ivan Bornyakov]

On Mon, Dec 26, 2022 at 08:24:06PM +0200, Ilpo Järvinen wrote:
> On Mon, 26 Dec 2022, Ivan Bornyakov wrote:
> 
> > Original busy loop with retries count in mpf_poll_status() is not too
> > reliable, as it takes different times on different systems. Replace it
> > with read_poll_timeout() macro.
> > 
> > Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
> > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> > ---
> >  drivers/fpga/microchip-spi.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> > index e72fedd93a27..f3ddfab87499 100644
> > --- a/drivers/fpga/microchip-spi.c
> > +++ b/drivers/fpga/microchip-spi.c
> > @@ -6,6 +6,7 @@
> >  #include <asm/unaligned.h>
> >  #include <linux/delay.h>
> >  #include <linux/fpga/fpga-mgr.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> >  #include <linux/spi/spi.h>
> > @@ -33,7 +34,7 @@
> >  
> >  #define	MPF_BITS_PER_COMPONENT_SIZE	22
> >  
> > -#define	MPF_STATUS_POLL_RETRIES		10000
> > +#define	MPF_STATUS_POLL_TIMEOUT		(2 * USEC_PER_SEC)
> >  #define	MPF_STATUS_BUSY			BIT(0)
> >  #define	MPF_STATUS_READY		BIT(1)
> >  #define	MPF_STATUS_SPI_VIOLATION	BIT(2)
> > @@ -197,21 +198,17 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
> >  /* Poll HW status until busy bit is cleared and mask bits are set. */
> >  static int mpf_poll_status(struct mpf_priv *priv, u8 mask)
> >  {
> > -	int status, retries = MPF_STATUS_POLL_RETRIES;
> > +	int ret, status;
> >  
> > -	while (retries--) {
> > -		status = mpf_read_status(priv);
> > -		if (status < 0)
> > -			return status;
> > -
> > -		if (status & MPF_STATUS_BUSY)
> > -			continue;
> > -
> > -		if (!mask || (status & mask))
> > -			return status;
> > -	}
> > +	ret = read_poll_timeout(mpf_read_status, status,
> > +				(status < 0) ||
> > +				(!(status & MPF_STATUS_BUSY) &&
> > +				 (!mask || (status & mask))),
> 
> I'm just noting that this code does not match function's comment 
> (neither pre nor post diff code). The comment claims "mask bits are set" 
> but the code tests "any mask bit set". 
> 
> However, currently it causes no issue because only MPF_STATUS_READY is 
> passed and it's BIT(1).
> 
> I think the condition matching to the comment would be this (IMHO, it is 
> also slightly simpler because !mask doesn't need to be special cased):
> 
> ((status & (MPF_STATUS_BUSY | mask)) == mask)
> 
> (But not necessarily to be changed in this patch.)
> 
> -- 
>  i.
> 

Agree. Thank you for pointing out!

> > +				0, MPF_STATUS_POLL_TIMEOUT, false, priv);
> > +	if (ret < 0)
> > +		return ret;
> >  
> > -	return -EBUSY;
> > +	return status;
> >  }
> >  
> >  static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t buf_size)
> > 
>