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

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

[Icenowy Zheng]

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

Check XCH bit when polling for transfer finish.

Signed-off-by: Icenowy Zheng <uwu@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);
-- 
2.36.0

Re: [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@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@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);

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

[Icenowy Zheng]

在 2022-07-11星期一的 00:03 +0100，Andre Przywara写道：
> On Tue, 28 Jun 2022 14:49:24 +0800
> Icenowy Zheng <uwu@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.

Ooops... It sounds like the SPI framework is broken?

> 
> > Check XCH bit when polling for transfer finish.
> 
> 
> > 
> > Signed-off-by: Icenowy Zheng <uwu@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);
> 

Re: [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@icenowy.me> wrote:

Hi Icenowy,

many thanks for the patch!

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

So as mentioned in the other reply, there are still issues in the SPI
driver, some problems being more general.
However this patch looks right, and seems like the more robust solution
than counting bytes in the FIFO, so I will take it.

> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Applied to sunxi/master.

Thanks,
Andre

> ---
>  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);