[U-Boot] Fix read error in fsl_qspi driver
Hi all,
The following patchset will fix QSPI flash read error discovered on i.MX7D Sabre eval system. Could you please check and apply to upcoming v2019.07 release?
Best regards, Thomas Schaefer
[PATCH 1/2] drivers/spi: fsl_qspi: fix read timeout [PATCH 2/2] drivers/spi: fsl_qspi: improve timeout calculation
During QSPI reads, current is_controller_busy function sporadically fails with -ETIMEDOUT due to fixed number of 5 test loops. Using timer functions to wait 1000 us instead will fix this.
Signed-off-by: Thomas Schaefer thomas.schaefer@kontron.com --- drivers/spi/fsl_qspi.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 1598c4f698..2c5937509f 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -152,7 +152,7 @@ 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 long timeout = timer_get_us() + 1000;
do { val = qspi_read32(priv->flags, &priv->regs->sr); @@ -160,10 +160,9 @@ static inline int is_controller_busy(const struct fsl_qspi_priv *priv) if ((~val & mask) == mask) return 0;
- udelay(1); - } while (--retry); - - return -ETIMEDOUT; + if (timer_get_us() > timeout ) + return -ETIMEDOUT; + } while (1); }
/* QSPI support swapping the flash read/write data
On Mon, Jul 1, 2019 at 12:38 PM Thomas Schaefer thomas.schaefer@kontron.com wrote:
During QSPI reads, current is_controller_busy function sporadically fails with -ETIMEDOUT due to fixed number of 5 test loops. Using timer functions to wait 1000 us instead will fix this.
Signed-off-by: Thomas Schaefer thomas.schaefer@kontron.com
Thanks for the fix:
Reviewed-by: Fabio Estevam festevam@gmail.com
On Mon, Jul 1, 2019 at 9:08 PM Thomas Schaefer thomas.schaefer@kontron.com wrote:
During QSPI reads, current is_controller_busy function sporadically fails with -ETIMEDOUT due to fixed number of 5 test loops. Using timer functions to wait 1000 us instead will fix this.
Signed-off-by: Thomas Schaefer thomas.schaefer@kontron.com
Applied to u-boot-spi/master
Use readl_poll_timeout instead of explicit calculation
Signed-off-by: Thomas Schaefer thomas.schaefer@kontron.com --- drivers/spi/fsl_qspi.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 2c5937509f..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,19 +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 long timeout = timer_get_us() + 1000; + 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; - - if (timer_get_us() > timeout ) - return -ETIMEDOUT; - } while (1); + return readl_poll_timeout(&priv->regs->sr, val, !(val & mask), 1000); }
/* QSPI support swapping the flash read/write data
On Mon, Jul 1, 2019 at 12:37 PM Thomas Schaefer thomas.schaefer@kontron.com wrote:
Use readl_poll_timeout instead of explicit calculation
Signed-off-by: Thomas Schaefer thomas.schaefer@kontron.com
Reviewed-by: Fabio Estevam festevam@gmail.com
On Mon, Jul 1, 2019 at 9:07 PM Thomas Schaefer thomas.schaefer@kontron.com wrote:
Use readl_poll_timeout instead of explicit calculation
Signed-off-by: Thomas Schaefer thomas.schaefer@kontron.com
drivers/spi/fsl_qspi.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 2c5937509f..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,19 +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 long timeout = timer_get_us() + 1000;
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);
This look like a BE check, which was not there before isn't it?
Von: Jagan Teki jagan@amarulasolutions.com Gesendet: Dienstag, 2. Juli 2019 13:12
On Mon, Jul 1, 2019 at 9:07 PM Thomas Schaefer thomas.schaefer@kontron.com wrote:
Use readl_poll_timeout instead of explicit calculation
Signed-off-by: Thomas Schaefer thomas.schaefer@kontron.com
drivers/spi/fsl_qspi.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 2c5937509f..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,19 +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 long timeout = timer_get_us() + 1000;
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);This look like a BE check, which was not there before isn't it?
Hi Jagan,
The previous implementation was using the qspi_read32 function, that implemented this endianess checking. As the readl_poll_timeout macro finally ends with
#define __arch_getl(a) (*(volatile unsigned int *)(a))
in arch/arm/include/asm/io.h. When swapping the 'mask' in case of BE, the result is the same as when using qspi_read32 before.
However, I did not test this as the QSPI_FLAG_REGMAP_ENDIAN_BIG is not set on i.MX7 systems.
Also I found that current linux kernel does this BE check (negated LE check) with the 'mask' variable in drivers/spi/spi-fsl-qspi.c
if (!q->devtype_data->little_endian) mask = (u32)cpu_to_be32(mask);
Best reards, Thomas
On Tue, Jul 2, 2019 at 5:13 PM Thomas Schaefer Thomas.Schaefer@kontron.com wrote:
Von: Jagan Teki jagan@amarulasolutions.com Gesendet: Dienstag, 2. Juli 2019 13:12
On Mon, Jul 1, 2019 at 9:07 PM Thomas Schaefer thomas.schaefer@kontron.com wrote:
Use readl_poll_timeout instead of explicit calculation
Signed-off-by: Thomas Schaefer thomas.schaefer@kontron.com
drivers/spi/fsl_qspi.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 2c5937509f..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,19 +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 long timeout = timer_get_us() + 1000;
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);This look like a BE check, which was not there before isn't it?
Hi Jagan,
The previous implementation was using the qspi_read32 function, that implemented this endianess checking. As the readl_poll_timeout macro finally ends with
#define __arch_getl(a) (*(volatile unsigned int *)(a))
in arch/arm/include/asm/io.h. When swapping the 'mask' in case of BE, the result is the same as when using qspi_read32 before.
Okay, it was supported before. thanks for the clarity.
participants (4)
-
Fabio Estevam -
Jagan Teki -
Thomas Schaefer -
Thomas Schaefer