[PATCH] spi: xilinx_spi: fix premature transfer termination at low SCK

[PATCH] spi: xilinx_spi: fix premature transfer termination at low SCK

[Suraj Kakade]

start_transfer() asserts SPICR_MASTER_INHIBIT as soon as SPISR_TX_EMPTY
is seen. SPISR_TX_EMPTY only indicates that the TX FIFO has drained;
the final byte may still be in the shift pipeline and actively
transmitting on the SPI bus. At low SCK frequencies the gap between
FIFO-empty and the last bit finishing its shift-out becomes wide
enough that MASTER_INHIBIT gets asserted before the transfer actually
completes, truncating the last byte and leaving the clock generation
logic in an error state.

Switch to polling IPISR_DTR_EMPTY instead, which is only set once the
last byte has genuinely finished shifting out to the slave, and only
assert MASTER_INHIBIT afterwards. Clear any stale DTR_EMPTY before
starting a transfer and after detecting completion, since IPISR is
toggle-on-write. This fix works correctly at both high and low SCK
frequencies.

Fixes: 0c0de58f7b30 ("spi: xilinx_spi: Modify transfer logic xilinx_spi_xfer() function")
Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
Signed-off-by: Suraj Kakade <suraj.hanumantkakade@amd.com>
---
 drivers/spi/xilinx_spi.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
index b2af17ebae9..011bbc93076 100644
--- a/drivers/spi/xilinx_spi.c
+++ b/drivers/spi/xilinx_spi.c
@@ -53,6 +53,8 @@
 #define SPISR_RX_FULL		BIT(1)
 #define SPISR_RX_EMPTY		BIT(0)
 
+#define IPISR_DTR_EMPTY		BIT(2)
+
 /* SPI Data Transmit Register (spidtr), [1] p12, [2] p12 */
 #define SPIDTR_8BIT_MASK	GENMASK(7, 0)
 #define SPIDTR_16BIT_MASK	GENMASK(15, 0)
@@ -256,6 +258,13 @@ static int start_transfer(struct udevice *dev, const void *dout, void *din, u32
 		reg = readl(&regs->spicr) | SPICR_MASTER_INHIBIT;
 		writel(reg, &regs->spicr);
 		count = xilinx_spi_fill_txfifo(bus, txp, txbytes);
+		/*
+		 * Clear any stale DTR_EMPTY before starting. IPISR is
+		 * toggle-on-write, so only write (toggle) the bit when it is
+		 * actually set, otherwise the write would spuriously set it.
+		 */
+		if (readl(&regs->ipisr) & IPISR_DTR_EMPTY)
+			writel(IPISR_DTR_EMPTY, &regs->ipisr);
 		/* Enable master transaction */
 		reg = readl(&regs->spicr) & ~SPICR_MASTER_INHIBIT;
 		writel(reg, &regs->spicr);
@@ -263,12 +272,22 @@ static int start_transfer(struct udevice *dev, const void *dout, void *din, u32
 		if (txp)
 			txp += count;
 
-		ret = wait_for_bit_le32(&regs->spisr, SPISR_TX_EMPTY, true,
+		/*
+		 * Wait for the transfer to fully complete before inhibiting
+		 * the master. The final byte may still be in the shift
+		 * pipeline. Poll IPISR_DTR_EMPTY, which is set only once the
+		 * last byte has been shifted out to the slave. Asserting
+		 * MASTER_INHIBIT before this stops SCK and truncates the last
+		 * byte, which is observable at low SCK frequencies.
+		 */
+		ret = wait_for_bit_le32(&regs->ipisr, IPISR_DTR_EMPTY, true,
 					XILINX_SPISR_TIMEOUT, false);
 		if (ret < 0) {
 			printf("XILSPI error: Xfer timeout\n");
 			return ret;
 		}
+		/* IPISR is toggle-on-write; write 1 to clear DTR_EMPTY */
+		writel(IPISR_DTR_EMPTY, &regs->ipisr);
 
 		reg = readl(&regs->spicr) | SPICR_MASTER_INHIBIT;
 		writel(reg, &regs->spicr);
-- 
2.43.7

Re: [PATCH] spi: xilinx_spi: fix premature transfer termination at low SCK

[Michal Simek]

On 8/13/26 14:06, Suraj Kakade wrote:
> start_transfer() asserts SPICR_MASTER_INHIBIT as soon as SPISR_TX_EMPTY
> is seen. SPISR_TX_EMPTY only indicates that the TX FIFO has drained;
> the final byte may still be in the shift pipeline and actively
> transmitting on the SPI bus. At low SCK frequencies the gap between
> FIFO-empty and the last bit finishing its shift-out becomes wide
> enough that MASTER_INHIBIT gets asserted before the transfer actually
> completes, truncating the last byte and leaving the clock generation
> logic in an error state.
> 
> Switch to polling IPISR_DTR_EMPTY instead, which is only set once the
> last byte has genuinely finished shifting out to the slave, and only
> assert MASTER_INHIBIT afterwards. Clear any stale DTR_EMPTY before
> starting a transfer and after detecting completion, since IPISR is
> toggle-on-write. This fix works correctly at both high and low SCK
> frequencies.
> 
> Fixes: 0c0de58f7b30 ("spi: xilinx_spi: Modify transfer logic xilinx_spi_xfer() function")
> Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
> Signed-off-by: Suraj Kakade <suraj.hanumantkakade@amd.com>
> ---
>   drivers/spi/xilinx_spi.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> index b2af17ebae9..011bbc93076 100644
> --- a/drivers/spi/xilinx_spi.c
> +++ b/drivers/spi/xilinx_spi.c
> @@ -53,6 +53,8 @@
>   #define SPISR_RX_FULL		BIT(1)
>   #define SPISR_RX_EMPTY		BIT(0)
>   
> +#define IPISR_DTR_EMPTY		BIT(2)
> +
>   /* SPI Data Transmit Register (spidtr), [1] p12, [2] p12 */
>   #define SPIDTR_8BIT_MASK	GENMASK(7, 0)
>   #define SPIDTR_16BIT_MASK	GENMASK(15, 0)
> @@ -256,6 +258,13 @@ static int start_transfer(struct udevice *dev, const void *dout, void *din, u32
>   		reg = readl(&regs->spicr) | SPICR_MASTER_INHIBIT;
>   		writel(reg, &regs->spicr);
>   		count = xilinx_spi_fill_txfifo(bus, txp, txbytes);
> +		/*
> +		 * Clear any stale DTR_EMPTY before starting. IPISR is
> +		 * toggle-on-write, so only write (toggle) the bit when it is
> +		 * actually set, otherwise the write would spuriously set it.
> +		 */
> +		if (readl(&regs->ipisr) & IPISR_DTR_EMPTY)
> +			writel(IPISR_DTR_EMPTY, &regs->ipisr);
>   		/* Enable master transaction */
>   		reg = readl(&regs->spicr) & ~SPICR_MASTER_INHIBIT;
>   		writel(reg, &regs->spicr);
> @@ -263,12 +272,22 @@ static int start_transfer(struct udevice *dev, const void *dout, void *din, u32
>   		if (txp)
>   			txp += count;
>   
> -		ret = wait_for_bit_le32(&regs->spisr, SPISR_TX_EMPTY, true,
> +		/*
> +		 * Wait for the transfer to fully complete before inhibiting
> +		 * the master. The final byte may still be in the shift
> +		 * pipeline. Poll IPISR_DTR_EMPTY, which is set only once the
> +		 * last byte has been shifted out to the slave. Asserting
> +		 * MASTER_INHIBIT before this stops SCK and truncates the last
> +		 * byte, which is observable at low SCK frequencies.
> +		 */
> +		ret = wait_for_bit_le32(&regs->ipisr, IPISR_DTR_EMPTY, true,
>   					XILINX_SPISR_TIMEOUT, false);
>   		if (ret < 0) {
>   			printf("XILSPI error: Xfer timeout\n");
>   			return ret;
>   		}
> +		/* IPISR is toggle-on-write; write 1 to clear DTR_EMPTY */
> +		writel(IPISR_DTR_EMPTY, &regs->ipisr);
>   
>   		reg = readl(&regs->spicr) | SPICR_MASTER_INHIBIT;
>   		writel(reg, &regs->spicr);

Applied.
M