[PATCH sunxi/next] spi: sunxi: use XCH status to detect in-progress transfer
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);
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);
在 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);
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);
participants (2)
-
Andre Przywara -
Icenowy Zheng