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 timedout 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 timedout 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 ;-)