From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Mon, 21 Dec 2015 11:16:41 +0100
Subject: [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
In-Reply-To: <CAK7LNAT9nJrd9+akzLvd0gZwyNGZq+S_iRLrAztKaueEquWA1A@mail.gmail.com>
References: <1450580348-6243-1-git-send-email-marex@denx.de>
 <CAK7LNAT9nJrd9+akzLvd0gZwyNGZq+S_iRLrAztKaueEquWA1A@mail.gmail.com>
Message-ID: <201512211116.41880.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Monday, December 21, 2015 at 09:40:16 AM, Masahiro Yamada wrote:
> Hi Marek,

Hi,

> 2015-12-20 11:59 GMT+09:00 Marek Vasut <marex@denx.de>:
> > This particular unbounded loop in the Denali NAND reset routine can
> > lead to a system hang in case neither of the Timeout and Completed
> > bits get set.
> > 
> > Refactor the code a bit so it's readable and implement timer so the
> > loop is bounded instead. This way the complete hang can be prevented
> > even if the NAND fails to reset.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: Scott Wood <scottwood@freescale.com>
> > ---
> > 
> >  drivers/mtd/nand/denali.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> > index 192be7d..8a8cca9 100644
> > --- a/drivers/mtd/nand/denali.c
> > +++ b/drivers/mtd/nand/denali.c
> > @@ -199,6 +199,7 @@ static void reset_bank(struct denali_nand_info
> > *denali)
> > 
> >  static uint32_t denali_nand_reset(struct denali_nand_info *denali)
> >  {
> >  
> >         int i;
> > 
> > +       u32 start, reg;
> > 
> >         for (i = 0; i < denali->max_banks; i++)
> >         
> >                 writel(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
> > 
> > @@ -206,12 +207,25 @@ static uint32_t denali_nand_reset(struct
> > denali_nand_info *denali)
> > 
> >         for (i = 0; i < denali->max_banks; i++) {
> >         
> >                 writel(1 << i, denali->flash_reg + DEVICE_RESET);
> > 
> > -               while (!(readl(denali->flash_reg + INTR_STATUS(i)) &
> > -                       (INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT)))
> > -                       if (readl(denali->flash_reg + INTR_STATUS(i)) &
> > -                               INTR_STATUS__TIME_OUT)
> > -                               debug("NAND Reset operation timed out on
> > bank" -                                     " %d\n", i);
> > +
> > +               start = get_timer(0);
> > +               while (1) {
> > +                       reg = readl(denali->flash_reg + INTR_STATUS(i));
> > +                       if (reg & INTR_STATUS__TIME_OUT) {
> > +                               debug("NAND Reset operation timed out on
> > bank %d\n", +                                     i);
> > +                               break;
> > +                       }
> > +
> > +                       /* Reset completed and did not time out, all
> > good. */ +                       if (reg & INTR_STATUS__RST_COMP)
> > +                               break;
> > +
> > +                       if (get_timer(start) > (CONFIG_SYS_HZ / 1000)) {
> > +                               debug("%s: Reset timed out!\n",
> > __func__); +                               break;
> > +                       }
> > +               }
> 
> I feel it is too much here
> about using get_timer() & CONFIG_SYS_HZ.
> 
> How about iterating udelay(1) up to reasonable times?

The get_timer() provides more precise delay , in this case, it's 1 sec .
Using just udelay() doesn't provide such precise control over the delay.
Moreover, I believe the get_timer() method is the one agreed upon for
implementing delays.

> See the  wait_for_irq() function in this file.

I'd like to convert this one to wait_for_bit() once that hits mainline.