[PATCH sunxi/next] spi: sunxi: use XCH status to detect in-progress transfer

[Andre Przywara]

On Tue, 28 Jun 2022 14:49:24 +0800
Icenowy Zheng <uwu at icenowy.me> wrote:

Hi Icenowy,

> The current detection of RX FIFO depth seems to be not reliable, and
> XCH will self-clear when a transfer is done.

many thanks for sending this, indeed what I put in -next is broken,
probably for everything except the F1C100 ;-)

Digging a bit deeper this gets more interesting, though:
I chased the issue down to my very first commit, that is (now properly!)
setting the SPI bus frequency in the SPI controller's CCR register. It
turns out that there are more issues in this driver, which lead to an
actual frequency limit of 1 MHz[1]. So my commit now actually programs
this value, and apparently it's too slow(?) for the code? Raising the
default 1 to 4 MHz makes it work again (even without your patch). The
previous timeout is generous, though, but by looking at the FIFO status
register it just seemed to be stuck after clocking out one byte only,
with the RX buf staying at 0. Reading FSR after your new loop reveals
that the condition holds (RX FIFO level == nbytes), so there is
something quite weird going on. Without your patch, but with some
udelay(1000) after(!) the loop it also works, interestingly.

As for the actual code change: looking at the XCH bit is probably
indeed the most robust and clever method of checking for the end of a
transfer, so I am tempted to take this change. However there are more
things broken, apparently, and I would like to get to the bottom of
those issues, before trying to paper over them.

Cheers,
Andre

[1] The driver (as most U-Boot SPI drivers, actually) tries to read
spi-max-frequency from the SPI's *controller* DT node, although this is
a SPI *slave* property. It (correctly) doesn't find anything in there,
so falls back to some (assumed) safe value of 1 MHz.

> Check XCH bit when polling for transfer finish.


> 
> Signed-off-by: Icenowy Zheng <uwu at icenowy.me>
> ---
>  drivers/spi/spi-sunxi.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
> index 2f33337725..a424c6a98e 100644
> --- a/drivers/spi/spi-sunxi.c
> +++ b/drivers/spi/spi-sunxi.c
> @@ -83,7 +83,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  #endif
>  #define SUN4I_SPI_MIN_RATE		3000
>  #define SUN4I_SPI_DEFAULT_RATE		1000000
> -#define SUN4I_SPI_TIMEOUT_US		1000000
> +#define SUN4I_SPI_TIMEOUT_MS		1000
>  
>  #define SPI_REG(priv, reg)		((priv)->base + \
>  					(priv)->variant->regs[reg])
> @@ -326,7 +326,6 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  	struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
>  
>  	u32 len = bitlen / 8;
> -	u32 rx_fifocnt;
>  	u8 nbytes;
>  	int ret;
>  
> @@ -364,13 +363,10 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  		setbits_le32(SPI_REG(priv, SPI_TCR),
>  			     SPI_BIT(priv, SPI_TCR_XCH));
>  
> -		/* Wait till RX FIFO to be empty */
> -		ret = readl_poll_timeout(SPI_REG(priv, SPI_FSR),
> -					 rx_fifocnt,
> -					 (((rx_fifocnt &
> -					 SPI_BIT(priv, SPI_FSR_RF_CNT_MASK)) >>
> -					 SUN4I_FIFO_STA_RF_CNT_BITS) >= nbytes),
> -					 SUN4I_SPI_TIMEOUT_US);
> +		/* Wait for the transfer to be done */
> +		ret = wait_for_bit_le32((const void *)SPI_REG(priv, SPI_TCR),
> +					SPI_BIT(priv, SPI_TCR_XCH),
> +					false, SUN4I_SPI_TIMEOUT_MS, false);
>  		if (ret < 0) {
>  			printf("ERROR: sun4i_spi: Timeout transferring data\n");
>  			sun4i_spi_set_cs(bus, slave_plat->cs, false);