Hi Daniil On 5/24/21 2:53 PM, Daniil Stas wrote: > On Mon, 24 May 2021 09:40:05 +0200 > Patrice CHOTARD <patrice.chot...@foss.st.com> wrote: > >> Hi Daniil >> >> On 5/24/21 12:24 AM, Daniil Stas wrote: >>> TCF flag only means that all data was sent to FIFO. To check if the >>> data was sent out of FIFO we should also wait for the BUSY flag to >>> be cleared. Otherwise there is a race condition which can lead to >>> inability to write short (one byte long) data. >>> >>> Signed-off-by: Daniil Stas <daniil.s...@posteo.net> >>> Cc: Patrick Delaunay <patrick.delau...@foss.st.com> >>> Cc: Patrice Chotard <patrice.chot...@foss.st.com> >>> --- >>> drivers/spi/stm32_qspi.c | 29 +++++++++++++++-------------- >>> 1 file changed, 15 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/spi/stm32_qspi.c b/drivers/spi/stm32_qspi.c >>> index 4acc9047b9..8f4aabc3d1 100644 >>> --- a/drivers/spi/stm32_qspi.c >>> +++ b/drivers/spi/stm32_qspi.c >>> @@ -148,23 +148,24 @@ static int _stm32_qspi_wait_cmd(struct >>> stm32_qspi_priv *priv, const struct spi_mem_op *op) >>> { >>> u32 sr; >>> - int ret; >>> - >>> - if (!op->data.nbytes) >>> - return _stm32_qspi_wait_for_not_busy(priv); >>> + int ret = 0; >>> >>> - ret = readl_poll_timeout(&priv->regs->sr, sr, >>> - sr & STM32_QSPI_SR_TCF, >>> - STM32_QSPI_CMD_TIMEOUT_US); >>> - if (ret) { >>> - log_err("cmd timeout (stat:%#x)\n", sr); >>> - } else if (readl(&priv->regs->sr) & STM32_QSPI_SR_TEF) { >>> - log_err("transfer error (stat:%#x)\n", sr); >>> - ret = -EIO; >>> + if (op->data.nbytes) { >>> + ret = readl_poll_timeout(&priv->regs->sr, sr, >>> + sr & STM32_QSPI_SR_TCF, >>> + >>> STM32_QSPI_CMD_TIMEOUT_US); >>> + if (ret) { >>> + log_err("cmd timeout (stat:%#x)\n", sr); >>> + } else if (readl(&priv->regs->sr) & >>> STM32_QSPI_SR_TEF) { >>> + log_err("transfer error (stat:%#x)\n", sr); >>> + ret = -EIO; >>> + } >>> + /* clear flags */ >>> + writel(STM32_QSPI_FCR_CTCF | STM32_QSPI_FCR_CTEF, >>> &priv->regs->fcr); } >>> >>> - /* clear flags */ >>> - writel(STM32_QSPI_FCR_CTCF | STM32_QSPI_FCR_CTEF, >>> &priv->regs->fcr); >>> + if (!ret) >>> + ret = _stm32_qspi_wait_for_not_busy(priv); >>> >>> return ret; >>> } >>> >> >> Have you got a simple test to reproduce the described race condition ? >> >> Thanks >> Patrice > > Hi, Patrice > > I found this issue on an stm32mp153 based board. > To reproduce it you need to set qspi peripheral clock to a low > value (for example 24 MHz). > Then you can test it in the u-boot console: > > STM32MP> clk dump > Clocks: > ... > - CK_PER : 24 MHz > ... > - QSPI(10) => parent CK_PER(30) > ... > > STM32MP> sf probe > SF: Detected w25q32jv with page size 256 Bytes, erase size 64 KiB, total 4 MiB > STM32MP> sf erase 0x00300000 +1 > SF: 65536 bytes @ 0x300000 Erased: OK > STM32MP> sf read 0xc4100000 0x300000 10 > device 0 offset 0x300000, size 0x10 > SF: 16 bytes @ 0x300000 Read: OK > STM32MP> md.b 0xc4100000 > c4100000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ > ... > STM32MP> mw.b 0xc4200000 55 > STM32MP> sf write 0xc4200000 0x00300000 1 > device 0 offset 0x300000, size 0x1 > SF: 1 bytes @ 0x300000 Written: OK > STM32MP> sf read 0xc4100000 0x00300000 10 > device 0 offset 0x300000, size 0x10 > SF: 16 bytes @ 0x300000 Read: OK > STM32MP> md.b 0xc4100000 > c4100000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ > ... > > > With my patch applied the last command result would be: > STM32MP> md.b 0xc4100000 > c4100000: 55 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff U............... > > Thanks, > Daniil >
Thanks for the detailed informations, i also reproduced this issue on a stm32mp157c-ev1 board. Reviewed-by: Patrice Chotard <patrice.chot...@foss.st.com> Patrice