[PATCH] spi: stm32_qspi: Fix short data write operation

[Patrice CHOTARD]

Hi Daniil

On 5/24/21 2:53 PM, Daniil Stas wrote:
> On Mon, 24 May 2021 09:40:05 +0200
> Patrice CHOTARD <patrice.chotard at 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.stas at posteo.net>
>>> Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
>>> Cc: Patrice Chotard <patrice.chotard at 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.chotard at foss.st.com>

Patrice