Hello Eugen
here is the updated patch
Am Mo., 14. Juli 2025 um 22:33 Uhr schrieb Eugen Hristev <
eugen.hris...@linaro.org>:
> Hello Ramin,
>
> Thank you for your patch.
>
> Can you please change the subject to have "spi: atmel_qspi: ..."
>
> On 7/13/25 19:08, Ramin Moussavi wrote:
> > In atmel_qspi_transfer(), the status register is polled with:
> >
> > imr = QSPI_SR_INSTRE | QSPI_SR_CSR;
> > return readl_poll_timeout(aq->regs + QSPI_SR, sr,
> > (sr & imr) == imr,
> > ATMEL_QSPI_TIMEOUT);
> >
> > However, this is racy: QSPI_SR_INSTRE can be set before QSPI_SR_CSR,
> > and will then be cleared by the read. If that happens, the condition
> > "(sr & imr) == imr" can never be true, and the function times out.
> >
> > This race condition is avoided in at91bootstrap by accumulating the
> > status bits across reads until both bits have been observed:
> >
> > /* Poll INSTruction End and Chip Select Rise flags. */
> > imr = (QSPI_SR_INSTRE | QSPI_SR_CSR);
> > sr = 0;
> > do {
> > udelay(1);
> > sr |= qspi_readl(qspi, QSPI_SR) & imr;
> > } while ((--timeout) && (sr != imr));
> >
> > Update U-Boot's atmel_qspi_transfer() to use the same pattern,
> > ensuring that both flags are observed even if they are not set
> > simultaneously.
> >
> > Signed-off-by: Ramin Seyed-Mousavi <lordras...@gmail.com>
> > ---
> > drivers/spi/atmel-quadspi.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> > index 8aa7a83aef4..fba32876f8f 100644
> > --- a/drivers/spi/atmel-quadspi.c
> > +++ b/drivers/spi/atmel-quadspi.c
> > @@ -616,6 +616,8 @@ static int atmel_qspi_transfer(struct atmel_qspi *aq,
> > const struct spi_mem_op *op, u32 offset)
> > {
> > u32 sr, imr;
> > + uint32_t val = 0;
> > + unsigned long timeout;
> >
> > /* Skip to the final steps if there is no data */
> > if (op->data.nbytes) {
> > @@ -636,8 +638,19 @@ static int atmel_qspi_transfer(struct atmel_qspi
> *aq,
> >
> > /* Poll INSTruction End and Chip Select Rise flags. */
> > imr = QSPI_SR_INSTRE | QSPI_SR_CSR;
> > - return readl_poll_timeout(aq->regs + QSPI_SR, sr, (sr & imr) ==
> imr,
> > - ATMEL_QSPI_TIMEOUT);
> > +
> > + timeout = timer_get_us() + ATMEL_QSPI_TIMEOUT;
> > + while(1){
>
> There is a missing space, can you please run checkpatch tool and fix the
> warnings given ?
>
> > + val |= readl( aq->regs + QSPI_SR ) & imr;
> > + if( ( val & imr ) == imr ){
>
> Remove unnecessary brackets please, also spacing is odd
>
> > + return 0;
> > + }
> > +
> > + if ( time_after(timer_get_us(), timeout)) {
> Same here
> > + return -ETIMEDOUT;
> > + }
> > + }
> > +
> > }
> >
> > static int atmel_qspi_sama7g5_set_cfg(struct atmel_qspi *aq,
>
>
> Eugen
>
From f801dc67950f9c06d64c5a25f1fd2cb377c04919 Mon Sep 17 00:00:00 2001
From: Ramin Moussavi <lordras...@gmail.com>
Date: Sun, 13 Jul 2025 17:43:31 +0200
Subject: [PATCH] arm: spi: atmel_qspi: fix race condition in transfer
completion check
In atmel_qspi_transfer(), the status register is polled with:
imr = QSPI_SR_INSTRE | QSPI_SR_CSR;
return readl_poll_timeout(aq->regs + QSPI_SR, sr,
(sr & imr) == imr,
ATMEL_QSPI_TIMEOUT);
However, this is racy: QSPI_SR_INSTRE can be set before QSPI_SR_CSR,
and will then be cleared by the read. If that happens, the condition
"(sr & imr) == imr" can never be true, and the function times out.
This race condition is avoided in at91bootstrap by accumulating the
status bits across reads until both bits have been observed:
/* Poll INSTruction End and Chip Select Rise flags. */
imr = (QSPI_SR_INSTRE | QSPI_SR_CSR);
sr = 0;
do {
udelay(1);
sr |= qspi_readl(qspi, QSPI_SR) & imr;
} while ((--timeout) && (sr != imr));
Update U-Boot's atmel_qspi_transfer() to use the same pattern,
ensuring that both flags are observed even if they are not set
simultaneously.
Signed-off-by: Ramin Moussavi <lordras...@gmail.com>
---
drivers/spi/atmel-quadspi.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 8aa7a83aef4..0f9ce409a0c 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -616,6 +616,8 @@ static int atmel_qspi_transfer(struct atmel_qspi *aq,
const struct spi_mem_op *op, u32 offset)
{
u32 sr, imr;
+ u32 val = 0;
+ unsigned long timeout;
/* Skip to the final steps if there is no data */
if (op->data.nbytes) {
@@ -636,8 +638,16 @@ static int atmel_qspi_transfer(struct atmel_qspi *aq,
/* Poll INSTruction End and Chip Select Rise flags. */
imr = QSPI_SR_INSTRE | QSPI_SR_CSR;
- return readl_poll_timeout(aq->regs + QSPI_SR, sr, (sr & imr) == imr,
- ATMEL_QSPI_TIMEOUT);
+
+ timeout = timer_get_us() + ATMEL_QSPI_TIMEOUT;
+ while (1) {
+ val |= readl(aq->regs + QSPI_SR) & imr;
+ if ((val & imr) == imr)
+ return 0;
+
+ if (time_after(timer_get_us(), timeout))
+ return -ETIMEDOUT;
+ }
}
static int atmel_qspi_sama7g5_set_cfg(struct atmel_qspi *aq,
--
2.50.1
