| Message ID | 20220628064924.390103-1-uwu@icenowy.me |
|---|---|
| State | Accepted |
| Delegated to: | Andre Przywara |
| Headers | show |
| Series | [sunxi/next] spi: sunxi: use XCH status to detect in-progress transfer | expand |
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);
在 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); > if (ret < 0) { > printf("ERROR: sun4i_spi: Timeout transferring data\n"); > sun4i_spi_set_cs(bus, slave_plat->cs, false);
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);
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(-)