[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Peter Tyser]

Without the timeout present an infinite loop can occur if the
NAND device is broken or not present.

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
 drivers/mtd/nand/nand_base.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index bfa7874..a2656eb 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -75,6 +75,17 @@
 #include <jffs2/jffs2.h>
 #endif
 
+/*
+ * CONFIG_SYS_NAND_RESET_CNT is used as a timeout mechanism when resetting
+ * a flash.  NAND flash is initialized prior to interrupts so standard timers
+ * can't be used.  CONFIG_SYS_NAND_RESET_CNT should be set to a value
+ * which is greater than (max NAND reset time / NAND status read time).
+ * A conservative default of 200000 (500 us / 25 ns) is used as a default.
+ */
+#ifndef CONFIG_SYS_NAND_RESET_CNT
+#define CONFIG_SYS_NAND_RESET_CNT 200000
+#endif
+
 /* Define default oob placement schemes for large and small page devices */
 static struct nand_ecclayout nand_oob_8 = {
 	.eccbytes = 3,
@@ -524,6 +535,7 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 {
 	register struct nand_chip *chip = mtd->priv;
 	int ctrl = NAND_CTRL_CLE | NAND_CTRL_CHANGE;
+	uint32_t rst_sts_cnt = CONFIG_SYS_NAND_RESET_CNT;
 
 	/*
 	 * Write out the command to the device.
@@ -590,7 +602,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 			       NAND_CTRL_CLE | NAND_CTRL_CHANGE);
 		chip->cmd_ctrl(mtd,
 			       NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
-		while (!(chip->read_byte(mtd) & NAND_STATUS_READY)) ;
+		while (!(chip->read_byte(mtd) & NAND_STATUS_READY) &&
+			(rst_sts_cnt--));
 		return;
 
 		/* This applies to read commands */
@@ -626,6 +639,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 			    int column, int page_addr)
 {
 	register struct nand_chip *chip = mtd->priv;
+	uint32_t rst_sts_cnt = CONFIG_SYS_NAND_RESET_CNT;
 
 	/* Emulate NAND_CMD_READOOB */
 	if (command == NAND_CMD_READOOB) {
@@ -696,7 +710,8 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 			       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
 		chip->cmd_ctrl(mtd, NAND_CMD_NONE,
 			       NAND_NCE | NAND_CTRL_CHANGE);
-		while (!(chip->read_byte(mtd) & NAND_STATUS_READY)) ;
+		while (!(chip->read_byte(mtd) & NAND_STATUS_READY) &&
+			(rst_sts_cnt--));
 		return;
 
 	case NAND_CMD_RNDOUT:
-- 
1.6.0.2.GIT

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Scott Wood]

Peter Tyser wrote:
> +/*
> + * CONFIG_SYS_NAND_RESET_CNT is used as a timeout mechanism when resetting
> + * a flash.  NAND flash is initialized prior to interrupts so standard timers
> + * can't be used.  CONFIG_SYS_NAND_RESET_CNT should be set to a value
> + * which is greater than (max NAND reset time / NAND status read time).
> + * A conservative default of 200000 (500 us / 25 ns) is used as a default.
> + */
> +#ifndef CONFIG_SYS_NAND_RESET_CNT
> +#define CONFIG_SYS_NAND_RESET_CNT 200000
> +#endif

Where does 25 ns come from?  Should the timeout be in terms of real time 
rather than iterations (we use get_ticks() for this purpose in 
fsl_elbc_nand.c)?

-Scott

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Peter Tyser]

On Wed, 2009-02-04 at 13:54 -0600, Scott Wood wrote:
> Peter Tyser wrote:
> > +/*
> > + * CONFIG_SYS_NAND_RESET_CNT is used as a timeout mechanism when resetting
> > + * a flash.  NAND flash is initialized prior to interrupts so standard timers
> > + * can't be used.  CONFIG_SYS_NAND_RESET_CNT should be set to a value
> > + * which is greater than (max NAND reset time / NAND status read time).
> > + * A conservative default of 200000 (500 us / 25 ns) is used as a default.
> > + */
> > +#ifndef CONFIG_SYS_NAND_RESET_CNT
> > +#define CONFIG_SYS_NAND_RESET_CNT 200000
> > +#endif
> 
> Where does 25 ns come from?  Should the timeout be in terms of real time 
> rather than iterations (we use get_ticks() for this purpose in 
> fsl_elbc_nand.c)?

The 25ns was calculated based on the addition of trp and trhoh from the
Micron MT29F8G08 datasheet.  Based on the timing diagram for a "Read
Status" cycle I thought this would be the minimum cycle time needed to
read the chip's status.  Other chips (ST, Samsung) I glanced at had > 25
ns read status times as well.

I had tried using get_timer() (I believe nand_wait() would have been
perfect to use), but that didn't work due to interrupts being disabled
when NAND is probed.  I didn't consider using get_ticks()...  That seems
much better.  Is get_ticks() available for all platforms when NAND is
initialized?

Assuming get_ticks() is available for all platforms, would you prefer I:
1. re-do the patch using get_ticks()
2. update nand_wait() to use get_ticks instead of get_timer() and use it

Thanks,
Peter

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Scott Wood]

Peter Tyser wrote:
> The 25ns was calculated based on the addition of trp and trhoh from the
> Micron MT29F8G08 datasheet.  Based on the timing diagram for a "Read
> Status" cycle I thought this would be the minimum cycle time needed to
> read the chip's status.  Other chips (ST, Samsung) I glanced at had > 25
> ns read status times as well.
> 
> I had tried using get_timer() (I believe nand_wait() would have been
> perfect to use), but that didn't work due to interrupts being disabled
> when NAND is probed.  I didn't consider using get_ticks()...  That seems
> much better.  Is get_ticks() available for all platforms when NAND is
> initialized?

Probably, but who knows what weirdness is out there.

> Assuming get_ticks() is available for all platforms, would you prefer I:
> 1. re-do the patch using get_ticks()
> 2. update nand_wait() to use get_ticks instead of get_timer() and use it

#2 looks better.

-Scott

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Wolfgang Denk]

Dear Scott Wood,

In message <4989FE57.80404@freescale.com> you wrote:
> Peter Tyser wrote:
> > The 25ns was calculated based on the addition of trp and trhoh from the
> > Micron MT29F8G08 datasheet.  Based on the timing diagram for a "Read
> > Status" cycle I thought this would be the minimum cycle time needed to
> > read the chip's status.  Other chips (ST, Samsung) I glanced at had > 25
> > ns read status times as well.
> > 
> > I had tried using get_timer() (I believe nand_wait() would have been
> > perfect to use), but that didn't work due to interrupts being disabled
> > when NAND is probed.  I didn't consider using get_ticks()...  That seems
> > much better.  Is get_ticks() available for all platforms when NAND is
> > initialized?

get_ticks() not a public interface. It should not be used in any
common code.

Please use get_timer().

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You can do this in a number of ways.     IBM chose to do all of them.
Why do you find that funny?        -- D. Taylor, Computer Science 350

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Peter Tyser]

On Wed, 2009-02-04 at 22:22 +0100, Wolfgang Denk wrote:
> Dear Scott Wood,
> 
> In message <4989FE57.80404@freescale.com> you wrote:
> > Peter Tyser wrote:
> > > The 25ns was calculated based on the addition of trp and trhoh from the
> > > Micron MT29F8G08 datasheet.  Based on the timing diagram for a "Read
> > > Status" cycle I thought this would be the minimum cycle time needed to
> > > read the chip's status.  Other chips (ST, Samsung) I glanced at had > 25
> > > ns read status times as well.
> > > 
> > > I had tried using get_timer() (I believe nand_wait() would have been
> > > perfect to use), but that didn't work due to interrupts being disabled
> > > when NAND is probed.  I didn't consider using get_ticks()...  That seems
> > > much better.  Is get_ticks() available for all platforms when NAND is
> > > initialized?
> 
> get_ticks() not a public interface. It should not be used in any
> common code.
> 
> Please use get_timer().

The problem is that the NAND code is used prior to interrupts being
enabled, thus we can't use get_timer().  I used a hokey delay based on
(read times * number of iterations).  Whats worse, my hokey loop or
get_ticks()?

Thanks,
Peter

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Wolfgang Denk]

Dear Peter Tyser,

In message <1233782967.7067.338.camel@localhost.localdomain> you wrote:
>
> > get_ticks() not a public interface. It should not be used in any
> > common code.
> > 
> > Please use get_timer().

Ah, I see.

> The problem is that the NAND code is used prior to interrupts being
> enabled, thus we can't use get_timer().  I used a hokey delay based on
> (read times * number of iterations).  Whats worse, my hokey loop or
> get_ticks()?

I can't tell. Thing is that get_ticks() may or may not exist. I don't
think all architectures have it. I don't see it for ixp, leon*, mcf*,
microblaze, mips, nios*, ...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"There is nothing new under the sun, but there are lots of old things
we don't know yet."                                  - Ambrose Bierce

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Scott Wood]

Wolfgang Denk wrote:
> get_ticks() not a public interface. It should not be used in any
> common code.

Then what is it doing in include/common.h?

> Please use get_timer().

But as was pointed out, that depends on interrupts, and thus does not 
work until very late in the boot process.

-Scott

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Wolfgang Denk]

Dear Scott,

In message <498A0917.8080404@freescale.com> you wrote:
>
> > get_ticks() not a public interface. It should not be used in any
> > common code.
> 
> Then what is it doing in include/common.h?
> 
> > Please use get_timer().
> 
> But as was pointed out, that depends on interrupts, and thus does not 
> work until very late in the boot process.

I see.

But get_ticks() is an internal interface that may, or may not be
preset. There are architectures where it does not exist.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If I ever needed a brain transplant, I'd choose a teenager's  because
I'd want a brain that had never been used.

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Peter Tyser]

Hi Scott,

On Wed, 2009-02-04 at 22:54 +0100, Wolfgang Denk wrote:
> Dear Scott,
> 
> In message <498A0917.8080404@freescale.com> you wrote:
> >
> > > get_ticks() not a public interface. It should not be used in any
> > > common code.
> > 
> > Then what is it doing in include/common.h?
> > 
> > > Please use get_timer().
> > 
> > But as was pointed out, that depends on interrupts, and thus does not 
> > work until very late in the boot process.
> 
> I see.
> 
> But get_ticks() is an internal interface that may, or may not be
> preset. There are architectures where it does not exist.

Since get_ticks() and get_timer() are not available, is the patch (hokey
as it is) acceptable?

Thanks,
Peter

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Scott Wood]

Peter Tyser wrote:
> Hi Scott,
> 
> On Wed, 2009-02-04 at 22:54 +0100, Wolfgang Denk wrote:
>> Dear Scott,
>>
>> In message <498A0917.8080404@freescale.com> you wrote:
>>>> get_ticks() not a public interface. It should not be used in any
>>>> common code.
>>> Then what is it doing in include/common.h?
>>>
>>>> Please use get_timer().
>>> But as was pointed out, that depends on interrupts, and thus does not 
>>> work until very late in the boot process.
>> I see.
>>
>> But get_ticks() is an internal interface that may, or may not be
>> preset. There are architectures where it does not exist.
> 
> Since get_ticks() and get_timer() are not available, is the patch (hokey
> as it is) acceptable?

Yes, I'll apply it.

-Scott

[U-Boot] [PATCH] nand_base: Add timeout for NAND reset command

[Scott Wood]

On Wed, Feb 04, 2009 at 01:47:22PM -0600, Peter Tyser wrote:
> Without the timeout present an infinite loop can occur if the
> NAND device is broken or not present.
> 
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>

Applied to u-boot-nand-flash.

-Scott