* [PATCH v3 0/3] Reliability improvements for Microchip MPF FPGA manager @ 2022-12-27 10:04 Ivan Bornyakov 2022-12-27 10:04 ` [PATCH v3 1/3] fpga: microchip-spi: move SPI I/O buffers out of stack Ivan Bornyakov ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Ivan Bornyakov @ 2022-12-27 10:04 UTC (permalink / raw) To: Conor Dooley, Moritz Fischer, Wu Hao, Xu Yilun, Tom Rix, Ilpo Järvinen Cc: Ivan Bornyakov, linux-fpga, linux-kernel, system 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 [https://lore.kernel.org/linux-fpga/20221226142326.8111-1-i.bornyakov@metrotek.ru/] v3: * fix polling stop condition in mpf_poll_status() as Ilpo suggested. 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 | 137 ++++++++++++++++++----------------- 1 file changed, 72 insertions(+), 65 deletions(-) -- 2.38.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/3] fpga: microchip-spi: move SPI I/O buffers out of stack 2022-12-27 10:04 [PATCH v3 0/3] Reliability improvements for Microchip MPF FPGA manager Ivan Bornyakov @ 2022-12-27 10:04 ` Ivan Bornyakov 2022-12-28 14:42 ` Conor Dooley 2022-12-27 10:04 ` [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way Ivan Bornyakov 2022-12-27 10:04 ` [PATCH v3 3/3] fpga: microchip-spi: separate data frame write routine Ivan Bornyakov 2 siblings, 1 reply; 8+ messages in thread From: Ivan Bornyakov @ 2022-12-27 10:04 UTC (permalink / raw) To: Conor Dooley, Moritz Fischer, Wu Hao, Xu Yilun, Tom Rix, Ilpo Järvinen Cc: Ivan Bornyakov, linux-fpga, linux-kernel, system 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/3] fpga: microchip-spi: move SPI I/O buffers out of stack 2022-12-27 10:04 ` [PATCH v3 1/3] fpga: microchip-spi: move SPI I/O buffers out of stack Ivan Bornyakov @ 2022-12-28 14:42 ` Conor Dooley 0 siblings, 0 replies; 8+ messages in thread From: Conor Dooley @ 2022-12-28 14:42 UTC (permalink / raw) To: Ivan Bornyakov Cc: Conor Dooley, Moritz Fischer, Wu Hao, Xu Yilun, Tom Rix, Ilpo Järvinen, linux-fpga, linux-kernel, system [-- Attachment #1: Type: text/plain, Size: 8345 bytes --] Hey Ivan, Given the time of year, I don't have my proper setup to actually go an test this (or the other patches). On Tue, Dec 27, 2022 at 01:04:48PM +0300, Ivan Bornyakov wrote: > 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> The changes look fine though... Acked-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor. > --- > 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 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way 2022-12-27 10:04 [PATCH v3 0/3] Reliability improvements for Microchip MPF FPGA manager Ivan Bornyakov 2022-12-27 10:04 ` [PATCH v3 1/3] fpga: microchip-spi: move SPI I/O buffers out of stack Ivan Bornyakov @ 2022-12-27 10:04 ` Ivan Bornyakov 2022-12-27 10:22 ` Ilpo Järvinen 2022-12-28 14:50 ` Conor Dooley 2022-12-27 10:04 ` [PATCH v3 3/3] fpga: microchip-spi: separate data frame write routine Ivan Bornyakov 2 siblings, 2 replies; 8+ messages in thread From: Ivan Bornyakov @ 2022-12-27 10:04 UTC (permalink / raw) To: Conor Dooley, Moritz Fischer, Wu Hao, Xu Yilun, Tom Rix, Ilpo Järvinen Cc: Ivan Bornyakov, linux-fpga, linux-kernel, system 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. While at it, fix polling stop condition to met function's original intention declared in the comment. The issue with original polling stop condition is that it stops if any of mask bits is set, while intention was to stop if all mask bits is set. This was not noticible because only MPF_STATUS_READY is passed as mask argument and it is BIT(1). Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager") Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> --- drivers/fpga/microchip-spi.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c index e72fedd93a27..8d1d9476d0cc 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,16 @@ 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)) == 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way 2022-12-27 10:04 ` [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way Ivan Bornyakov @ 2022-12-27 10:22 ` Ilpo Järvinen 2022-12-28 14:50 ` Conor Dooley 1 sibling, 0 replies; 8+ messages in thread From: Ilpo Järvinen @ 2022-12-27 10:22 UTC (permalink / raw) To: Ivan Bornyakov Cc: Conor Dooley, Moritz Fischer, Wu Hao, Xu Yilun, Tom Rix, linux-fpga, LKML, system [-- Attachment #1: Type: text/plain, Size: 2489 bytes --] On Tue, 27 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. > > While at it, fix polling stop condition to met function's original > intention declared in the comment. The issue with original polling stop > condition is that it stops if any of mask bits is set, while intention > was to stop if all mask bits is set. This was not noticible because only > MPF_STATUS_READY is passed as mask argument and it is BIT(1). > > Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager") > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> > --- > drivers/fpga/microchip-spi.c | 24 ++++++++++-------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c > index e72fedd93a27..8d1d9476d0cc 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,16 @@ 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)) == 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) Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> -- i. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way 2022-12-27 10:04 ` [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way Ivan Bornyakov 2022-12-27 10:22 ` Ilpo Järvinen @ 2022-12-28 14:50 ` Conor Dooley 1 sibling, 0 replies; 8+ messages in thread From: Conor Dooley @ 2022-12-28 14:50 UTC (permalink / raw) To: Ivan Bornyakov Cc: Conor Dooley, Moritz Fischer, Wu Hao, Xu Yilun, Tom Rix, Ilpo Järvinen, linux-fpga, linux-kernel, system [-- Attachment #1: Type: text/plain, Size: 2821 bytes --] On Tue, Dec 27, 2022 at 01:04:49PM +0300, 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. > > While at it, fix polling stop condition to met function's original > intention declared in the comment. The issue with original polling stop > condition is that it stops if any of mask bits is set, while intention > was to stop if all mask bits is set. This was not noticible because only > MPF_STATUS_READY is passed as mask argument and it is BIT(1). > > Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager") > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> > --- > drivers/fpga/microchip-spi.c | 24 ++++++++++-------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c > index e72fedd93a27..8d1d9476d0cc 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,16 @@ 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; > - } Would you mind moving the small comment before the function down to here and mentioning handling the failure case? The current implementation is a bit easier to understand than the new version that's wrapped in read_poll_timeout(). Otherwise, change seems like a good idea. Acked-by: Conor Dooley <conor.dooley@microchip.com> > + ret = read_poll_timeout(mpf_read_status, status, > + (status < 0) || > + ((status & (MPF_STATUS_BUSY | mask)) == mask), > + 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 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 3/3] fpga: microchip-spi: separate data frame write routine 2022-12-27 10:04 [PATCH v3 0/3] Reliability improvements for Microchip MPF FPGA manager Ivan Bornyakov 2022-12-27 10:04 ` [PATCH v3 1/3] fpga: microchip-spi: move SPI I/O buffers out of stack Ivan Bornyakov 2022-12-27 10:04 ` [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way Ivan Bornyakov @ 2022-12-27 10:04 ` Ivan Bornyakov 2022-12-28 14:54 ` Conor Dooley 2 siblings, 1 reply; 8+ messages in thread From: Ivan Bornyakov @ 2022-12-27 10:04 UTC (permalink / raw) To: Conor Dooley, Moritz Fischer, Wu Hao, Xu Yilun, Tom Rix, Ilpo Järvinen Cc: Ivan Bornyakov, linux-fpga, linux-kernel, system 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 8d1d9476d0cc..ea92e5d106fa 100644 --- a/drivers/fpga/microchip-spi.c +++ b/drivers/fpga/microchip-spi.c @@ -274,9 +274,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; @@ -287,19 +308,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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 3/3] fpga: microchip-spi: separate data frame write routine 2022-12-27 10:04 ` [PATCH v3 3/3] fpga: microchip-spi: separate data frame write routine Ivan Bornyakov @ 2022-12-28 14:54 ` Conor Dooley 0 siblings, 0 replies; 8+ messages in thread From: Conor Dooley @ 2022-12-28 14:54 UTC (permalink / raw) To: Ivan Bornyakov Cc: Conor Dooley, Moritz Fischer, Wu Hao, Xu Yilun, Tom Rix, Ilpo Järvinen, linux-fpga, linux-kernel, system [-- Attachment #1: Type: text/plain, Size: 2456 bytes --] On Tue, Dec 27, 2022 at 01:04:50PM +0300, Ivan Bornyakov wrote: > 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> Acked-by: Conor Dooley <conor.dooley@microchip.com> Without another user for the new function I'm not super pushed, but I suppose it does make things a little more readable. Thanks, Conor. > --- > 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 8d1d9476d0cc..ea92e5d106fa 100644 > --- a/drivers/fpga/microchip-spi.c > +++ b/drivers/fpga/microchip-spi.c > @@ -274,9 +274,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; > @@ -287,19 +308,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 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-12-28 14:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-27 10:04 [PATCH v3 0/3] Reliability improvements for Microchip MPF FPGA manager Ivan Bornyakov 2022-12-27 10:04 ` [PATCH v3 1/3] fpga: microchip-spi: move SPI I/O buffers out of stack Ivan Bornyakov 2022-12-28 14:42 ` Conor Dooley 2022-12-27 10:04 ` [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way Ivan Bornyakov 2022-12-27 10:22 ` Ilpo Järvinen 2022-12-28 14:50 ` Conor Dooley 2022-12-27 10:04 ` [PATCH v3 3/3] fpga: microchip-spi: separate data frame write routine Ivan Bornyakov 2022-12-28 14:54 ` Conor Dooley
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.