diff mbox series

[U-Boot] QSPI support on i.MX7D Sabre Eval Board

Message ID 25a89a15fd1344c2a31fd0d535ccc0cc@kontron.com
State Not Applicable
Delegated to: Stefano Babic
Headers show
Series [U-Boot] QSPI support on i.MX7D Sabre Eval Board | expand

Commit Message

Thomas Schaefer June 19, 2019, 1:04 p.m. UTC 
Hi all,

I've built current u-boot (v2019.07-rc4) with QSPI support for our i.MX7D Sabre eval system. I am using mx7dsabresd_qspi_defconfig.

The resulting u-boot fails to read from QSPI, i.e. the data read is not the same than that written before with a previous (v2018) version.

After some investigation I found that the 'is_controller_busy' function comes back with -ETIMEDOUT during read operation, because the retry count of 5 is too low.

To figure out how many retries are needed, I added some debug code as part of a while(1) loop to the is_controller_busy function:


On my MX7 eval system I found that a retry count between 73 and 147 is necessary to make sure that the controller will be ready before the next operation.

So from my point of view we have 2 options to fix this issue:

- increase retry from 5 to 200
- wait in an endless loop (while (1) ) and return only when the controller is ready (which means that the board will hang if the controller doesn't come back for some other reason.

What do you think?

Best regards,
Thomas Schäfer
SW Design Engineer

Kontron - An S&T Company
Heinrich-Barth-Straße 1-1a | 66115 Saarbrücken | Germany

thomas.schaefer@kontron.com

http://www.kontron.com
Website | Blog | Twitter | LinkedIn | YouTube | Facebook 

Kontron Europe GmbH
Die gesetzlichen Pflichtangaben finden Sie hier. 
Please find our mandatory legal statements here. 
Mit dem Öffnen dieses E-Mails stimmen Sie Kontrons Richtlinien zur elektronischen Kommunikation zu.
By opening this email you are agreeing to Kontron's Electronic Communications Policy.

Comments

Fabio Estevam June 19, 2019, 1:14 p.m. UTC | #1 
Hi Thomas,

On Wed, Jun 19, 2019 at 10:04 AM Thomas Schaefer
<Thomas.Schaefer@kontron.com> wrote:
>
> Hi all,
>
> I've built current u-boot (v2019.07-rc4) with QSPI support for our i.MX7D Sabre eval system. I am using mx7dsabresd_qspi_defconfig.
>
> The resulting u-boot fails to read from QSPI, i.e. the data read is not the same than that written before with a previous (v2018) version.
>
> After some investigation I found that the 'is_controller_busy' function comes back with -ETIMEDOUT during read operation, because the retry count of 5 is too low.
>
> To figure out how many retries are needed, I added some debug code as part of a while(1) loop to the is_controller_busy function:
>
> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
> index 1598c4f698..fa5b7a29f2 100644
> --- a/drivers/spi/fsl_qspi.c
> +++ b/drivers/spi/fsl_qspi.c
> @@ -152,16 +152,20 @@ static inline int is_controller_busy(const struct fsl_qspi_priv *priv)
>         u32 val;
>         const u32 mask = QSPI_SR_BUSY_MASK | QSPI_SR_AHB_ACC_MASK |
>                          QSPI_SR_IP_ACC_MASK;
> -       unsigned int retry = 5;
> +       unsigned int retry = 0;
>
>         do {
>                 val = qspi_read32(priv->flags, &priv->regs->sr);
>
> -               if ((~val & mask) == mask)
> +               if ((~val & mask) == mask) {
> +                       if (retry > 5)
> +                               printf("%s: timeout! retry = %d\n", __func__, retry);
>                         return 0;
> +               }
>
>                 udelay(1);
> -       } while (--retry);
> +               retry++;
> +       } while (1);
>
>         return -ETIMEDOUT;
>  }
>
> On my MX7 eval system I found that a retry count between 73 and 147 is necessary to make sure that the controller will be ready before the next operation.
>
> So from my point of view we have 2 options to fix this issue:
>
> - increase retry from 5 to 200
> - wait in an endless loop (while (1) ) and return only when the controller is ready (which means that the board will hang if the controller doesn't come back for some other reason.
>
> What do you think?

I think that readl_poll_timeout() could be used here as it will
timeout if the QSPI controller is not ready.

Also, could you check how this is handled in the fsl qspi driver in the kernel?
Thomas Schaefer June 19, 2019, 4:10 p.m. UTC | #2 
Hi Fabio,

> Hi Thomas,
> 
> On Wed, Jun 19, 2019 at 10:04 AM Thomas Schaefer <Thomas.Schaefer@kontron.com> wrote:
> >
> > Hi all,
> >
> > I've built current u-boot (v2019.07-rc4) with QSPI support for our i.MX7D Sabre eval system. I am using mx7dsabresd_qspi_defconfig.
> >
> > The resulting u-boot fails to read from QSPI, i.e. the data read is not the same than that written before with a previous (v2018) version.
> >
> > After some investigation I found that the 'is_controller_busy' function comes back with -ETIMEDOUT during read operation, because the retry count >of 5 is too low.
> >
> > To figure out how many retries are needed, I added some debug code as part of a while(1) loop to the is_controller_busy function:
> >
> > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 
> > 1598c4f698..fa5b7a29f2 100644
> > --- a/drivers/spi/fsl_qspi.c
> > +++ b/drivers/spi/fsl_qspi.c
> > @@ -152,16 +152,20 @@ static inline int is_controller_busy(const struct fsl_qspi_priv *priv)
> >         u32 val;
> >         const u32 mask = QSPI_SR_BUSY_MASK | QSPI_SR_AHB_ACC_MASK |
> >                          QSPI_SR_IP_ACC_MASK;
> > -       unsigned int retry = 5;
> > +       unsigned int retry = 0;
> >
> >         do {
> >                 val = qspi_read32(priv->flags, &priv->regs->sr);
> >
> > -               if ((~val & mask) == mask)
> > +               if ((~val & mask) == mask) {
> > +                       if (retry > 5)
> > +                               printf("%s: timeout! retry = %d\n", 
> > + __func__, retry);
> >                         return 0;
> > +               }
> >
> >                 udelay(1);
> > -       } while (--retry);
> > +               retry++;
> > +       } while (1);
> >
> >         return -ETIMEDOUT;
> >  }
> >
> > On my MX7 eval system I found that a retry count between 73 and 147 is necessary to make sure that the controller will be ready before the next operation.
> >
> > So from my point of view we have 2 options to fix this issue:
> >
> > - increase retry from 5 to 200
> > - wait in an endless loop (while (1) ) and return only when the controller is ready (which means that the board will hang if the controller doesn't come back for some other reason.
> >
> > What do you think?
>
> I think that readl_poll_timeout() could be used here as it will timeout if the QSPI controller is not ready.
>
> Also, could you check how this is handled in the fsl qspi driver in the kernel?

There is a similar readl_plll_tout call in 5.1.12 linux kernel driver (spi-fsl-qspi.c) with 1000us timeout:

/* wait for the controller being ready */
fsl_qspi_readl_poll_tout(q, base + QUADSPI_SR, (QUADSPI_SR_IP_ACC_MASK |
				 QUADSPI_SR_AHB_ACC_MASK), 10, 1000);

The fsl additions take concern about endianess before calling the general 'readl_poll_timeout' function.

Thomas
Thomas Schaefer June 24, 2019, 3:47 p.m. UTC | #3 
Hi all,


> Hi Fabio,
> 
> > Hi Thomas,
> > 
> > On Wed, Jun 19, 2019 at 10:04 AM Thomas Schaefer <Thomas.Schaefer@kontron.com> wrote:
> > >
> > > Hi all,
> > >
> > > I've built current u-boot (v2019.07-rc4) with QSPI support for our i.MX7D Sabre eval system. I am using mx7dsabresd_qspi_defconfig.
> > >
> > > The resulting u-boot fails to read from QSPI, i.e. the data read is not the same than that written before with a previous (v2018) version.
> > >
> > > After some investigation I found that the 'is_controller_busy' function comes back with -ETIMEDOUT during read operation, because the retry count >of 5 is too low.
> > >
> > > To figure out how many retries are needed, I added some debug code as part of a while(1) loop to the is_controller_busy function:
> > >
> > > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index
> > > 1598c4f698..fa5b7a29f2 100644
> > > --- a/drivers/spi/fsl_qspi.c
> > > +++ b/drivers/spi/fsl_qspi.c
> > > @@ -152,16 +152,20 @@ static inline int is_controller_busy(const struct fsl_qspi_priv *priv)
> > >         u32 val;
> > >         const u32 mask = QSPI_SR_BUSY_MASK | QSPI_SR_AHB_ACC_MASK |
> > >                          QSPI_SR_IP_ACC_MASK;
> > > -       unsigned int retry = 5;
> > > +       unsigned int retry = 0;
> > >
> > >         do {
> > >                 val = qspi_read32(priv->flags, &priv->regs->sr);
> > >
> > > -               if ((~val & mask) == mask)
> > > +               if ((~val & mask) == mask) {
> > > +                       if (retry > 5)
> > > +                               printf("%s: timeout! retry = %d\n", 
> > > + __func__, retry);
> > >                         return 0;
> > > +               }
> > >
> > >                 udelay(1);
> > > -       } while (--retry);
> > > +               retry++;
> > > +       } while (1);
> > >
> > >         return -ETIMEDOUT;
> > >  }
> > >
> > > On my MX7 eval system I found that a retry count between 73 and 147 is necessary to make sure that the controller will be ready before the next operation.
> > >
> > > So from my point of view we have 2 options to fix this issue:
> > >
> > > - increase retry from 5 to 200
> > > - wait in an endless loop (while (1) ) and return only when the controller is ready (which means that the board will hang if the controller doesn't come back for some other reason.
> > >
> > > What do you think?
> >
> > I think that readl_poll_timeout() could be used here as it will timeout if the QSPI controller is not ready.
> >
> > Also, could you check how this is handled in the fsl qspi driver in the kernel?
> 
> There is a similar readl_plll_tout call in 5.1.12 linux kernel driver (spi-fsl-qspi.c) with 1000us timeout:
> 
> /* wait for the controller being ready */ fsl_qspi_readl_poll_tout(q, base + QUADSPI_SR, (QUADSPI_SR_IP_ACC_MASK |
> 				 QUADSPI_SR_AHB_ACC_MASK), 10, 1000);
> 
> The fsl additions take concern about endianess before calling the general 'readl_poll_timeout' function.
> 
> Thomas
>

Following Fabio's suggestion I have prepared the following patch that fixes the QSPI read support issue of the fsl_qspi driver on my i.MX7D Sabre eval system. Could you please review/check this patch and introduce it into upcoming v2019.07 version?

Thanks, Thomas


From: Thomas Schaefer <thomas.schaefer@kontron.com>
Date: Mon, 24 Jun 2019 17:15:57 +0200
Subject: [PATCH] driver/spi: fsl_qspi: fix is_controller_busy check

Use readl_poll_timeout function to check for busy controller with
a timeout of 1000 us instead of 5 fixed test loops.

Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com>
---
 drivers/spi/fsl_qspi.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index 1598c4f698..41abe1996f 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -10,6 +10,7 @@
 #include <spi.h>
 #include <asm/io.h>
 #include <linux/sizes.h>
+#include <linux/iopoll.h>
 #include <dm.h>
 #include <errno.h>
 #include <watchdog.h>
@@ -150,20 +151,13 @@ static void qspi_write32(u32 flags, u32 *addr, u32 val)
 static inline int is_controller_busy(const struct fsl_qspi_priv *priv)
 {
        u32 val;
-       const u32 mask = QSPI_SR_BUSY_MASK | QSPI_SR_AHB_ACC_MASK |
-                        QSPI_SR_IP_ACC_MASK;
-       unsigned int retry = 5;
+       u32 mask = QSPI_SR_BUSY_MASK | QSPI_SR_AHB_ACC_MASK |
+                  QSPI_SR_IP_ACC_MASK;

-       do {
-               val = qspi_read32(priv->flags, &priv->regs->sr);
+       if (priv->flags & QSPI_FLAG_REGMAP_ENDIAN_BIG)
+               mask = (u32)cpu_to_be32(mask);

-               if ((~val & mask) == mask)
-                       return 0;
-
-               udelay(1);
-       } while (--retry);
-
-       return -ETIMEDOUT;
+       return readl_poll_timeout(&priv->regs->sr, val, !(val & mask), 1000);
 }

 /* QSPI support swapping the flash read/write data
--
2.11.0
Fabio Estevam June 24, 2019, 3:57 p.m. UTC | #4 
Hi Thomas,

On Mon, Jun 24, 2019 at 12:47 PM Thomas Schaefer
<Thomas.Schaefer@kontron.com> wrote:

> Following Fabio's suggestion I have prepared the following patch that fixes the QSPI read support issue of the fsl_qspi driver on my i.MX7D Sabre eval system. Could you please review/check this patch and introduce it into upcoming v2019.07 version?
>
> Thanks, Thomas
>
>
> From: Thomas Schaefer <thomas.schaefer@kontron.com>
> Date: Mon, 24 Jun 2019 17:15:57 +0200
> Subject: [PATCH] driver/spi: fsl_qspi: fix is_controller_busy check
>
> Use readl_poll_timeout function to check for busy controller with
> a timeout of 1000 us instead of 5 fixed test loops.
>
> Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com>

I think the change looks good, but I suggest to improve the commit log
by stating that with the current code you observe timeout during QSPI
read.

Please submit it as formal patch.

Thanks
diff mbox series

Patch

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index 1598c4f698..fa5b7a29f2 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -152,16 +152,20 @@  static inline int is_controller_busy(const struct fsl_qspi_priv *priv)
        u32 val;
        const u32 mask = QSPI_SR_BUSY_MASK | QSPI_SR_AHB_ACC_MASK |
                         QSPI_SR_IP_ACC_MASK;
-       unsigned int retry = 5;
+       unsigned int retry = 0;

        do {
                val = qspi_read32(priv->flags, &priv->regs->sr);

-               if ((~val & mask) == mask)
+               if ((~val & mask) == mask) {
+                       if (retry > 5)
+                               printf("%s: timeout! retry = %d\n", __func__, retry);
                        return 0;
+               }

                udelay(1);
-       } while (--retry);
+               retry++;
+       } while (1);

        return -ETIMEDOUT;
 }