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

[Michal Simek <michal.simek@amd.com>]

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