[U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop

[Masahiro Yamada]

Hi Marek,


2015-12-21 19:16 GMT+09:00 Marek Vasut <marex at 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 at 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 at denx.de>
>> > Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
>> > Cc: Scott Wood <scottwood at 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.


OK. Why do you want to wait precisely 1 sec.
Rationale for "1 sec", please?



> Moreover, I believe the get_timer() method is the one agreed upon for
> implementing delays.


You do not have to use CONFIG_SYS_HZ.

As commented in lib/timer.c,
get_timer() returns time in milliseconds.




>> See the  wait_for_irq() function in this file.
>
> I'd like to convert this one to wait_for_bit() once that hits mainline.


No justice for the conversion.

This function just waits long enough,
1 sec, 2 sec, or whatever.


-- 
Best Regards
Masahiro Yamada