[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Fabio Estevam]

The following hang is observed on a Hummingboard 2 MicroSOM
i2eX iMX6D - rev 1.3 with no eMMC populated on board:

U-Boot SPL 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
Trying to boot from MMC1

U-Boot 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)

CPU:   Freescale i.MX6D rev1.5 996 MHz (running at 792 MHz)
CPU:   Extended Commercial temperature grade (-20C to 105C) at 33C
Reset cause: POR
Board: MX6 HummingBoard2
DRAM:  1 GiB
MMC:   FSL_SDHC: 0, FSL_SDHC: 1
Loading Environment from MMC... *** Warning - bad CRC, using default environment

No panel detected: default to HDMI
Display: HDMI (1024x768)
In:    serial
Out:   serial
Err:   serial
---> hangs

which is caused by the following infinite loop inside esdhc_send_cmd_common()

	while (!(esdhc_read32(&regs->irqstat) & flags))
		;

Instead of looping forever, provide an exit path so that a timeout
error can be propagated in the case irqstat does not report
any interrupts, which may happen when no eMMC is populated on
board.

Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
Changes since v1:
- Jump to the label out on error path (Baruch).

 drivers/mmc/fsl_esdhc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 3cdfa7f..b8d0b00 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -396,6 +396,7 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
 	uint	irqstat;
 	u32	flags = IRQSTAT_CC | IRQSTAT_CTOE;
 	struct fsl_esdhc *regs = priv->esdhc_regs;
+	unsigned long start;
 
 #ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC111
 	if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
@@ -453,8 +454,13 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
 		flags = IRQSTAT_BRR;
 
 	/* Wait for the command to complete */
-	while (!(esdhc_read32(&regs->irqstat) & flags))
-		;
+	start = get_timer(0);
+	while (!(esdhc_read32(&regs->irqstat) & flags)) {
+		if (get_timer(start) > 1000) {
+			err = -ETIMEDOUT;
+			goto out;
+		}
+	}
 
 	irqstat = esdhc_read32(&regs->irqstat);
 
-- 
2.7.4

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Peter Robinson]

On Mon, Nov 19, 2018 at 12:31 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> The following hang is observed on a Hummingboard 2 MicroSOM
> i2eX iMX6D - rev 1.3 with no eMMC populated on board:
>
> U-Boot SPL 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> Trying to boot from MMC1
>
> U-Boot 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
>
> CPU:   Freescale i.MX6D rev1.5 996 MHz (running at 792 MHz)
> CPU:   Extended Commercial temperature grade (-20C to 105C) at 33C
> Reset cause: POR
> Board: MX6 HummingBoard2
> DRAM:  1 GiB
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> Loading Environment from MMC... *** Warning - bad CRC, using default environment
>
> No panel detected: default to HDMI
> Display: HDMI (1024x768)
> In:    serial
> Out:   serial
> Err:   serial
> ---> hangs
>
> which is caused by the following infinite loop inside esdhc_send_cmd_common()
>
>         while (!(esdhc_read32(&regs->irqstat) & flags))
>                 ;
>
> Instead of looping forever, provide an exit path so that a timeout
> error can be propagated in the case irqstat does not report
> any interrupts, which may happen when no eMMC is populated on
> board.
>
> Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
Tested-by: Peter Robinson <pbrobinson@gmail.com>

Boots fine on my i.MX6 Quad hummingboard2 with SOM rev 1.5 which
previously has the problem I reported back in 2018.09 series.

Peter

> ---
> Changes since v1:
> - Jump to the label out on error path (Baruch).
>
>  drivers/mmc/fsl_esdhc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index 3cdfa7f..b8d0b00 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -396,6 +396,7 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
>         uint    irqstat;
>         u32     flags = IRQSTAT_CC | IRQSTAT_CTOE;
>         struct fsl_esdhc *regs = priv->esdhc_regs;
> +       unsigned long start;
>
>  #ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC111
>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> @@ -453,8 +454,13 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
>                 flags = IRQSTAT_BRR;
>
>         /* Wait for the command to complete */
> -       while (!(esdhc_read32(&regs->irqstat) & flags))
> -               ;
> +       start = get_timer(0);
> +       while (!(esdhc_read32(&regs->irqstat) & flags)) {
> +               if (get_timer(start) > 1000) {
> +                       err = -ETIMEDOUT;
> +                       goto out;
> +               }
> +       }
>
>         irqstat = esdhc_read32(&regs->irqstat);
>
> --
> 2.7.4
>

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Ricardo Salveti]

Hi Fabio,

On Mon, Nov 19, 2018 at 10:31 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> The following hang is observed on a Hummingboard 2 MicroSOM
> i2eX iMX6D - rev 1.3 with no eMMC populated on board:
>
> U-Boot SPL 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> Trying to boot from MMC1
>
> U-Boot 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
>
> CPU:   Freescale i.MX6D rev1.5 996 MHz (running at 792 MHz)
> CPU:   Extended Commercial temperature grade (-20C to 105C) at 33C
> Reset cause: POR
> Board: MX6 HummingBoard2
> DRAM:  1 GiB
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> Loading Environment from MMC... *** Warning - bad CRC, using default environment
>
> No panel detected: default to HDMI
> Display: HDMI (1024x768)
> In:    serial
> Out:   serial
> Err:   serial
> ---> hangs
>
> which is caused by the following infinite loop inside esdhc_send_cmd_common()
>
>         while (!(esdhc_read32(&regs->irqstat) & flags))
>                 ;
>
> Instead of looping forever, provide an exit path so that a timeout
> error can be propagated in the case irqstat does not report
> any interrupts, which may happen when no eMMC is populated on
> board.
>
> Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes since v1:
> - Jump to the label out on error path (Baruch).
>
>  drivers/mmc/fsl_esdhc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index 3cdfa7f..b8d0b00 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -396,6 +396,7 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
>         uint    irqstat;
>         u32     flags = IRQSTAT_CC | IRQSTAT_CTOE;
>         struct fsl_esdhc *regs = priv->esdhc_regs;
> +       unsigned long start;
>
>  #ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC111
>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> @@ -453,8 +454,13 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
>                 flags = IRQSTAT_BRR;
>
>         /* Wait for the command to complete */
> -       while (!(esdhc_read32(&regs->irqstat) & flags))
> -               ;
> +       start = get_timer(0);
> +       while (!(esdhc_read32(&regs->irqstat) & flags)) {
> +               if (get_timer(start) > 1000) {
> +                       err = -ETIMEDOUT;
> +                       goto out;
> +               }
> +       }

This fixes the hang I was having, but adds a significant boot delay
because it needs to wait until it gets the timeout (1000). Is there a
better timeout value to be used here?

Thanks,
-- 
Ricardo Salveti de Araujo

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Ricardo Salveti]

On Mon, Nov 19, 2018 at 12:03 PM Ricardo Salveti <rsalveti@rsalveti.net> wrote:
>
> Hi Fabio,
>
> On Mon, Nov 19, 2018 at 10:31 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > The following hang is observed on a Hummingboard 2 MicroSOM
> > i2eX iMX6D - rev 1.3 with no eMMC populated on board:
> >
> > U-Boot SPL 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> > Trying to boot from MMC1
> >
> > U-Boot 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> >
> > CPU:   Freescale i.MX6D rev1.5 996 MHz (running at 792 MHz)
> > CPU:   Extended Commercial temperature grade (-20C to 105C) at 33C
> > Reset cause: POR
> > Board: MX6 HummingBoard2
> > DRAM:  1 GiB
> > MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> > Loading Environment from MMC... *** Warning - bad CRC, using default environment
> >
> > No panel detected: default to HDMI
> > Display: HDMI (1024x768)
> > In:    serial
> > Out:   serial
> > Err:   serial
> > ---> hangs
> >
> > which is caused by the following infinite loop inside esdhc_send_cmd_common()
> >
> >         while (!(esdhc_read32(&regs->irqstat) & flags))
> >                 ;
> >
> > Instead of looping forever, provide an exit path so that a timeout
> > error can be propagated in the case irqstat does not report
> > any interrupts, which may happen when no eMMC is populated on
> > board.
> >
> > Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
> > Signed-off-by: Fabio Estevam <festevam@gmail.com>
> > ---
> > Changes since v1:
> > - Jump to the label out on error path (Baruch).
> >
> >  drivers/mmc/fsl_esdhc.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> > index 3cdfa7f..b8d0b00 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc.c
> > @@ -396,6 +396,7 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
> >         uint    irqstat;
> >         u32     flags = IRQSTAT_CC | IRQSTAT_CTOE;
> >         struct fsl_esdhc *regs = priv->esdhc_regs;
> > +       unsigned long start;
> >
> >  #ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC111
> >         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> > @@ -453,8 +454,13 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
> >                 flags = IRQSTAT_BRR;
> >
> >         /* Wait for the command to complete */
> > -       while (!(esdhc_read32(&regs->irqstat) & flags))
> > -               ;
> > +       start = get_timer(0);
> > +       while (!(esdhc_read32(&regs->irqstat) & flags)) {
> > +               if (get_timer(start) > 1000) {
> > +                       err = -ETIMEDOUT;
> > +                       goto out;
> > +               }
> > +       }
>
> This fixes the hang I was having, but adds a significant boot delay
> because it needs to wait until it gets the timeout (1000). Is there a
> better timeout value to be used here?

Looking a bit further, this ends up adding a significant boot delay
because the mmc driver still tries to send another two additional
commands, so it ends executing this timeout logic 3 times in total.
Guess this can be improved at
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/mmc/mmc.c;h=d6b9cdc99229d8bfe0c8be3adfd6e5209a11601c;hb=HEAD#l2633
by either checking for mmc first or by simply just checking and
handling the return code from mmc_send_if_cond, but this could be
another patch.

Would still be nice to confirm if 1000 is the minimal timeout value to
be used here.

Thanks,
-- 
Ricardo Salveti de Araujo

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Fabio Estevam]

Hi Ricardo,

On Mon, Nov 19, 2018 at 12:25 PM Ricardo Salveti <rsalveti@rsalveti.net> wrote:

> Looking a bit further, this ends up adding a significant boot delay
> because the mmc driver still tries to send another two additional
> commands, so it ends executing this timeout logic 3 times in total.
> Guess this can be improved at
> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/mmc/mmc.c;h=d6b9cdc99229d8bfe0c8be3adfd6e5209a11601c;hb=HEAD#l2633
> by either checking for mmc first or by simply just checking and
> handling the return code from mmc_send_if_cond, but this could be
> another patch.

Yes, I think you are right.

Does only adding the error check below avoid the hang in your case?

        /* Test for SD version 2 */
         err = mmc_send_if_cond(mmc);
         if (err)
            return err;

Thanks

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Baruch Siach]

Hi Fabio,

On Mon, Nov 19, 2018 at 12:41:49PM -0200, Fabio Estevam wrote:
> On Mon, Nov 19, 2018 at 12:25 PM Ricardo Salveti <rsalveti@rsalveti.net> 
> wrote:
> 
> > Looking a bit further, this ends up adding a significant boot delay
> > because the mmc driver still tries to send another two additional
> > commands, so it ends executing this timeout logic 3 times in total.
> > Guess this can be improved at
> > http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/mmc/mmc.c;h=d6b9cdc99229d8bfe0c8be3adfd6e5209a11601c;hb=HEAD#l2633
> > by either checking for mmc first or by simply just checking and
> > handling the return code from mmc_send_if_cond, but this could be
> > another patch.
> 
> Yes, I think you are right.
> 
> Does only adding the error check below avoid the hang in your case?
> 
>         /* Test for SD version 2 */
>          err = mmc_send_if_cond(mmc);
>          if (err)
>             return err;

There used to be a similar check in the past. Commit f33cb34b3971dabe3 ("mmc: 
Remove return from mmc_init for non SD 2.0 compatible cards.") removed this 
check.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Ricardo Salveti]

On Mon, Nov 19, 2018 at 12:42 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Ricardo,
>
> On Mon, Nov 19, 2018 at 12:25 PM Ricardo Salveti <rsalveti@rsalveti.net> wrote:
>
> > Looking a bit further, this ends up adding a significant boot delay
> > because the mmc driver still tries to send another two additional
> > commands, so it ends executing this timeout logic 3 times in total.
> > Guess this can be improved at
> > http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/mmc/mmc.c;h=d6b9cdc99229d8bfe0c8be3adfd6e5209a11601c;hb=HEAD#l2633
> > by either checking for mmc first or by simply just checking and
> > handling the return code from mmc_send_if_cond, but this could be
> > another patch.
>
> Yes, I think you are right.
>
> Does only adding the error check below avoid the hang in your case?
>
>         /* Test for SD version 2 */
>          err = mmc_send_if_cond(mmc);
>          if (err)
>             return err;

You patch is still required as the hang I was getting was happening in
the while loop itself. Adding this change on top of your patch would
just reduce the delay as the code path would avoid another 2 extra
commands (both returning timeout).

The only issue, as pointed out by Baruch, is that we already had a
check that was ignoring the timeout here before, so I guess the
current behavior is correct/expected.

+1 for your patch as it is a good fix anyway and unblocks the boot
process on my som/board, even if it adds a longer boot delay.

Tested-by: Ricardo Salveti <rsalveti@rsalveti.net>

Thanks,
-- 
Ricardo Salveti de Araujo

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Fabio Estevam]

Hi Jaehoon,

On Mon, Nov 19, 2018 at 10:31 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> The following hang is observed on a Hummingboard 2 MicroSOM
> i2eX iMX6D - rev 1.3 with no eMMC populated on board:
>
> U-Boot SPL 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> Trying to boot from MMC1
>
> U-Boot 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
>
> CPU:   Freescale i.MX6D rev1.5 996 MHz (running at 792 MHz)
> CPU:   Extended Commercial temperature grade (-20C to 105C) at 33C
> Reset cause: POR
> Board: MX6 HummingBoard2
> DRAM:  1 GiB
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> Loading Environment from MMC... *** Warning - bad CRC, using default environment
>
> No panel detected: default to HDMI
> Display: HDMI (1024x768)
> In:    serial
> Out:   serial
> Err:   serial
> ---> hangs
>
> which is caused by the following infinite loop inside esdhc_send_cmd_common()
>
>         while (!(esdhc_read32(&regs->irqstat) & flags))
>                 ;
>
> Instead of looping forever, provide an exit path so that a timeout
> error can be propagated in the case irqstat does not report
> any interrupts, which may happen when no eMMC is populated on
> board.
>
> Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

Could this one be applied to 2019.01?

[U-Boot] [PATCH v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Fabio Estevam]

Hi Tom/Stefano,

On Sat, Dec 8, 2018 at 12:58 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Jaehoon,
>
> On Mon, Nov 19, 2018 at 10:31 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > The following hang is observed on a Hummingboard 2 MicroSOM
> > i2eX iMX6D - rev 1.3 with no eMMC populated on board:
> >
> > U-Boot SPL 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> > Trying to boot from MMC1
> >
> > U-Boot 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> >
> > CPU:   Freescale i.MX6D rev1.5 996 MHz (running at 792 MHz)
> > CPU:   Extended Commercial temperature grade (-20C to 105C) at 33C
> > Reset cause: POR
> > Board: MX6 HummingBoard2
> > DRAM:  1 GiB
> > MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> > Loading Environment from MMC... *** Warning - bad CRC, using default environment
> >
> > No panel detected: default to HDMI
> > Display: HDMI (1024x768)
> > In:    serial
> > Out:   serial
> > Err:   serial
> > ---> hangs
> >
> > which is caused by the following infinite loop inside esdhc_send_cmd_common()
> >
> >         while (!(esdhc_read32(&regs->irqstat) & flags))
> >                 ;
> >
> > Instead of looping forever, provide an exit path so that a timeout
> > error can be propagated in the case irqstat does not report
> > any interrupts, which may happen when no eMMC is populated on
> > board.
> >
> > Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
> > Signed-off-by: Fabio Estevam <festevam@gmail.com>
>
> Could this one be applied to 2019.01?

I haven't heard back from Jaehoon.

Can we have this one applied for 2019.01 in order to fix a regression?

Thanks

[U-Boot] [U-Boot, v2] mmc: fsl_esdhc: Avoid infinite loop in esdhc_send_cmd_common()

[Tom Rini]

On Mon, Nov 19, 2018 at 10:31:53AM -0200, Fabio Estevam wrote:

> The following hang is observed on a Hummingboard 2 MicroSOM
> i2eX iMX6D - rev 1.3 with no eMMC populated on board:
> 
> U-Boot SPL 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> Trying to boot from MMC1
> 
> U-Boot 2018.11+gf6206f8587 (Nov 16 2018 - 00:56:34 +0000)
> 
> CPU:   Freescale i.MX6D rev1.5 996 MHz (running at 792 MHz)
> CPU:   Extended Commercial temperature grade (-20C to 105C) at 33C
> Reset cause: POR
> Board: MX6 HummingBoard2
> DRAM:  1 GiB
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> Loading Environment from MMC... *** Warning - bad CRC, using default environment
> 
> No panel detected: default to HDMI
> Display: HDMI (1024x768)
> In:    serial
> Out:   serial
> Err:   serial

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190114/bc4637fd/attachment.sig>