From: Patrice CHOTARD <patrice.chotard@foss.st.com>
To: Daniil Stas <daniil.stas@posteo.net>
Cc: <u-boot@lists.denx.de>, Patrick Delaunay <patrick.delaunay@foss.st.com>
Subject: Re: [PATCH] spi: stm32_qspi: Fix short data write operation
Date: Fri, 18 Jun 2021 10:01:12 +0200 [thread overview]
Message-ID: <9c850021-d11a-9e28-6da6-44cf82c6e78e@foss.st.com> (raw)
In-Reply-To: <026719e7-bbb8-7fd8-bcf6-75a7be76f304@foss.st.com>
On 5/25/21 6:02 PM, Patrice CHOTARD wrote:
> 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@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@posteo.net>
>>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>> Cc: Patrice Chotard <patrice.chotard@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@foss.st.com>
>
> Patrice
>
>
Applied on u-boot-stm32/next
Thanks
next prev parent reply other threads:[~2021-06-18 8:01 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-23 22:24 [PATCH] net: dwc_eth_qos: Fix needless phy auto-negotiation restarts Daniil Stas
2021-05-23 22:24 ` [PATCH] spi: stm32_qspi: Fix short data write operation Daniil Stas
2021-05-24 7:40 ` Patrice CHOTARD
2021-05-24 12:53 ` Daniil Stas
2021-05-25 16:02 ` Patrice CHOTARD
2021-06-18 8:01 ` Patrice CHOTARD [this message]
2021-06-01 15:31 ` Patrick DELAUNAY
2021-05-24 8:30 ` [PATCH] net: dwc_eth_qos: Fix needless phy auto-negotiation restarts Patrice CHOTARD
2021-05-25 6:53 ` Ramon Fried
2021-06-01 15:26 ` Patrick DELAUNAY
2021-06-12 18:36 ` Ramon Fried
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9c850021-d11a-9e28-6da6-44cf82c6e78e@foss.st.com \
--to=patrice.chotard@foss.st.com \
--cc=daniil.stas@posteo.net \
--cc=patrick.delaunay@foss.st.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).