Message ID | 1450580348-6243-1-git-send-email-marex@denx.de |
---|---|
State | Deferred |
Delegated to: | Marek Vasut |
Headers | show |
Hi Marek, 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? See the wait_for_irq() function in this file.
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.
Hi Marek, 2015-12-21 19:16 GMT+09:00 Marek Vasut <marex@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. 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.
On Monday, December 21, 2015 at 11:32:09 AM, Masahiro Yamada wrote: > Hi Marek, > > 2015-12-21 19:16 GMT+09:00 Marek Vasut <marex@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. > > OK. Why do you want to wait precisely 1 sec. > Rationale for "1 sec", please? There isn't any, 1 second sounds about right for a timeout in my mind. > > 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. Ah, so you'd prefer just 1000 in there instead ? > >> 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. It'd better to have one timeout function than multiple implementation of the same thing in multiple drivers, that's all. > This function just waits long enough, > 1 sec, 2 sec, or whatever.
Hi Marek, 2015-12-21 19:45 GMT+09:00 Marek Vasut <marex@denx.de>: > On Monday, December 21, 2015 at 11:32:09 AM, Masahiro Yamada wrote: >> Hi Marek, >> >> 2015-12-21 19:16 GMT+09:00 Marek Vasut <marex@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. >> >> OK. Why do you want to wait precisely 1 sec. >> Rationale for "1 sec", please? > > There isn't any, 1 second sounds about right for a timeout in my mind. > >> > 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. > > Ah, so you'd prefer just 1000 in there instead ? Yes. >> >> 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. > > It'd better to have one timeout function than multiple implementation of the > same thing in multiple drivers, that's all. > >> This function just waits long enough, >> 1 sec, 2 sec, or whatever. I noticed one thing I had missed. While the CPU is stuck in udelay() function, it cannot check the register flag that might have been already set. This possibly wastes a small piece of time slice. So, I am OK with your way (and also with your next patch for the wait_for_bit() conversion, if you like). Go ahead!
On Monday, December 21, 2015 at 11:56:30 AM, Masahiro Yamada wrote: > Hi Marek, Hi! > 2015-12-21 19:45 GMT+09:00 Marek Vasut <marex@denx.de>: > > On Monday, December 21, 2015 at 11:32:09 AM, Masahiro Yamada wrote: > >> Hi Marek, > >> > >> 2015-12-21 19:16 GMT+09:00 Marek Vasut <marex@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. > >> > >> OK. Why do you want to wait precisely 1 sec. > >> Rationale for "1 sec", please? > > > > There isn't any, 1 second sounds about right for a timeout in my mind. > > > >> > 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. > > > > Ah, so you'd prefer just 1000 in there instead ? > > Yes. Roger, makes sense, will do. > >> >> 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. > > > > It'd better to have one timeout function than multiple implementation of > > the same thing in multiple drivers, that's all. > > > >> This function just waits long enough, > >> 1 sec, 2 sec, or whatever. > > I noticed one thing I had missed. > > While the CPU is stuck in udelay() function, > it cannot check the register flag that might have been already set. > This possibly wastes a small piece of time slice. Well yeah, but that's such a small timeslice that it's not very important. > So, I am OK with your way > (and also with your next patch for the wait_for_bit() conversion, if you > like). > > Go ahead! THanks ;-)
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; + } + } } for (i = 0; i < denali->max_banks; i++)
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(-)