[PATCH 1/6] nand_wait_ready timeout fix

[PATCH 1/6] nand_wait_ready timeout fix

[Matthieu CASTET]

nand_wait_ready timeout should not assume HZ=1000.
Make it independent of HZ value.

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
---
 drivers/mtd/nand/nand_base.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a46e9bb..a3c7fd3 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -512,7 +512,7 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
 void nand_wait_ready(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
-	unsigned long timeo = jiffies + 2;
+	unsigned long timeo = jiffies + (2 * HZ) / 1000;
 
 	/* 400ms timeout */
 	if (in_interrupt() || oops_in_progress)
-- 
1.7.5.4

[PATCH 2/6] nand_wait : warn if the nand is busy on exit

[Matthieu CASTET]

This patch allow to detect buggy driver/hardware with
bad RnB (dev_ready) management.
This check cost nothing and could help to detect bugs.

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
---
 drivers/mtd/nand/nand_base.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a3c7fd3..095dfea 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -885,6 +885,8 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 	led_trigger_event(nand_led_trigger, LED_OFF);
 
 	status = (int)chip->read_byte(mtd);
+	/* This can happen if in case of timeout or buggy dev_ready */
+	WARN_ON(!(status & NAND_STATUS_READY));
 	return status;
 }
 
-- 
1.7.5.4

Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit

[Artem Bityutskiy]

On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> This patch allow to detect buggy driver/hardware with
> bad RnB (dev_ready) management.
> This check cost nothing and could help to detect bugs.
> 
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> ---
>  drivers/mtd/nand/nand_base.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index a3c7fd3..095dfea 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -885,6 +885,8 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
>  	led_trigger_event(nand_led_trigger, LED_OFF);
>  
>  	status = (int)chip->read_byte(mtd);
> +	/* This can happen if in case of timeout or buggy dev_ready */
> +	WARN_ON(!(status & NAND_STATUS_READY));
>  	return status;

This seem to completely miss the chip->dev_ready != NULL case, e.g.,
piece of code above is like this

                while (time_before(jiffies, timeo)) {
                        if (chip->dev_ready) {
                                if (chip->dev_ready(mtd))
                                        break;
                        } else {
                                if (chip->read_byte(mtd) & NAND_STATUS_READY)
                                        break;
                        }
                        cond_resched();
                }

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit

[Matthieu CASTET]

Artem Bityutskiy a écrit :
> On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
>> This patch allow to detect buggy driver/hardware with
>> bad RnB (dev_ready) management.
>> This check cost nothing and could help to detect bugs.
>>
>> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
>> ---
>>  drivers/mtd/nand/nand_base.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index a3c7fd3..095dfea 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -885,6 +885,8 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
>>  	led_trigger_event(nand_led_trigger, LED_OFF);
>>  
>>  	status = (int)chip->read_byte(mtd);
>> +	/* This can happen if in case of timeout or buggy dev_ready */
>> +	WARN_ON(!(status & NAND_STATUS_READY));
>>  	return status;
> 
> This seem to completely miss the chip->dev_ready != NULL case, e.g.,
> piece of code above is like this
> 
>                 while (time_before(jiffies, timeo)) {
>                         if (chip->dev_ready) {
>                                 if (chip->dev_ready(mtd))
>                                         break;
>                         } else {
>                                 if (chip->read_byte(mtd) & NAND_STATUS_READY)
>                                         break;
>                         }
>                         cond_resched();
>                 }
> 
Sorry, I don't understand what you mean.

We don't care what's done in the loop (chip->dev_ready != NULL or, not). We only
check when the loop exit, that READY bit is set in the status.


Matthieu

Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit

[Artem Bityutskiy]

On Tue, 2011-06-28 at 17:03 +0200, Matthieu CASTET wrote:
> Artem Bityutskiy a écrit :
> > On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> >> This patch allow to detect buggy driver/hardware with
> >> bad RnB (dev_ready) management.
> >> This check cost nothing and could help to detect bugs.
> >>
> >> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> >> ---
> >>  drivers/mtd/nand/nand_base.c |    2 ++
> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index a3c7fd3..095dfea 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -885,6 +885,8 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> >>  	led_trigger_event(nand_led_trigger, LED_OFF);
> >>  
> >>  	status = (int)chip->read_byte(mtd);
> >> +	/* This can happen if in case of timeout or buggy dev_ready */
> >> +	WARN_ON(!(status & NAND_STATUS_READY));
> >>  	return status;
> > 
> > This seem to completely miss the chip->dev_ready != NULL case, e.g.,
> > piece of code above is like this
> > 
> >                 while (time_before(jiffies, timeo)) {
> >                         if (chip->dev_ready) {
> >                                 if (chip->dev_ready(mtd))
> >                                         break;
> >                         } else {
> >                                 if (chip->read_byte(mtd) & NAND_STATUS_READY)
> >                                         break;
> >                         }
> >                         cond_resched();
> >                 }
> > 
> Sorry, I don't understand what you mean.
> 
> We don't care what's done in the loop (chip->dev_ready != NULL or, not). We only
> check when the loop exit, that READY bit is set in the status.

Well, the logic is suspicious.

1. For NAND with chip->dev_ready != NULL, why NAND_STATUS_READY should
be set? We do not check for this in the loop.

2. For NAND with chip->dev_ready != NULL, if NAND_STATUS_READY has to be
set at the end, why wouldn't we drop this chip_ready part completely? We
could just loop while NAND_STATUS_READY is not set.

Isn't this strange?

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit

[Ivan Djelic]

On Tue, Jun 28, 2011 at 08:05:14PM +0100, Artem Bityutskiy wrote:
> On Tue, 2011-06-28 at 17:03 +0200, Matthieu CASTET wrote:
> > Artem Bityutskiy a écrit :
> > > On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> > >> This patch allow to detect buggy driver/hardware with
> > >> bad RnB (dev_ready) management.
> > >> This check cost nothing and could help to detect bugs.
> > >>
> > >> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> > >> ---
> > >>  drivers/mtd/nand/nand_base.c |    2 ++
> > >>  1 files changed, 2 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > >> index a3c7fd3..095dfea 100644
> > >> --- a/drivers/mtd/nand/nand_base.c
> > >> +++ b/drivers/mtd/nand/nand_base.c
> > >> @@ -885,6 +885,8 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> > >>  	led_trigger_event(nand_led_trigger, LED_OFF);
> > >>  
> > >>  	status = (int)chip->read_byte(mtd);
> > >> +	/* This can happen if in case of timeout or buggy dev_ready */
> > >> +	WARN_ON(!(status & NAND_STATUS_READY));
> > >>  	return status;
> > > 
> > > This seem to completely miss the chip->dev_ready != NULL case, e.g.,
> > > piece of code above is like this
> > > 
> > >                 while (time_before(jiffies, timeo)) {
> > >                         if (chip->dev_ready) {
> > >                                 if (chip->dev_ready(mtd))
> > >                                         break;
> > >                         } else {
> > >                                 if (chip->read_byte(mtd) & NAND_STATUS_READY)
> > >                                         break;
> > >                         }
> > >                         cond_resched();
> > >                 }
> > > 
> > Sorry, I don't understand what you mean.
> > 
> > We don't care what's done in the loop (chip->dev_ready != NULL or, not). We only
> > check when the loop exit, that READY bit is set in the status.
> 
> Well, the logic is suspicious.
> 
> 1. For NAND with chip->dev_ready != NULL, why NAND_STATUS_READY should
> be set? We do not check for this in the loop.
> 
> 2. For NAND with chip->dev_ready != NULL, if NAND_STATUS_READY has to be
> set at the end, why wouldn't we drop this chip_ready part completely? We
> could just loop while NAND_STATUS_READY is not set.
> 
> Isn't this strange?

Not really. There are 2 methods to wait for an erase/program command completion:

1. Wait until nand RnB pin goes high (that's what chip->dev_ready usually does)
2. Poll the device: send a status (0x70) command and read status byte in a loop
   until bit NAND_STATUS_READY is set

In all cases, you should send a status command after completion, to check if
the operation was successful. And if the operation completed, the status should
have bit NAND_STATUS_READY set.

Method 1 is optimal, you can often use an interrupt to signal completion, and
no cpu cycles are lost in a polling loop. But flaky hardware (bad pull-ups) or
bad gpio configuration can make it unreliable.

Method 2 is the safest, it always works, but it is less efficient.

Here, Matthieu wants to detect cases where method 1 is unreliable or a timeout
occurs (e.g. somebody forgot to put a nand device on the board :).
Both conditions are not expected on working hardware.

Another option (that I used on other systems) is to systematically perform
method 1 first (if chip->dev_ready != NULL), _then_ method 2.
If method 1 works as expected, then the polling loop in method 2 finishes
immediately because nand status already has bit NAND_STATUS_READY set.

--
Best Regards,

Ivan

Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit

[Artem Bityutskiy]

On Wed, 2011-06-29 at 15:59 +0200, Ivan Djelic wrote:
> On Tue, Jun 28, 2011 at 08:05:14PM +0100, Artem Bityutskiy wrote:
> > On Tue, 2011-06-28 at 17:03 +0200, Matthieu CASTET wrote:
> > > Artem Bityutskiy a écrit :
> > > > On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> > > >> This patch allow to detect buggy driver/hardware with
> > > >> bad RnB (dev_ready) management.
> > > >> This check cost nothing and could help to detect bugs.
> > > >>
> > > >> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> > > >> ---
> > > >>  drivers/mtd/nand/nand_base.c |    2 ++
> > > >>  1 files changed, 2 insertions(+), 0 deletions(-)
> > > >>
> > > >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > >> index a3c7fd3..095dfea 100644
> > > >> --- a/drivers/mtd/nand/nand_base.c
> > > >> +++ b/drivers/mtd/nand/nand_base.c
> > > >> @@ -885,6 +885,8 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> > > >>  	led_trigger_event(nand_led_trigger, LED_OFF);
> > > >>  
> > > >>  	status = (int)chip->read_byte(mtd);
> > > >> +	/* This can happen if in case of timeout or buggy dev_ready */
> > > >> +	WARN_ON(!(status & NAND_STATUS_READY));
> > > >>  	return status;
> > > > 
> > > > This seem to completely miss the chip->dev_ready != NULL case, e.g.,
> > > > piece of code above is like this
> > > > 
> > > >                 while (time_before(jiffies, timeo)) {
> > > >                         if (chip->dev_ready) {
> > > >                                 if (chip->dev_ready(mtd))
> > > >                                         break;
> > > >                         } else {
> > > >                                 if (chip->read_byte(mtd) & NAND_STATUS_READY)
> > > >                                         break;
> > > >                         }
> > > >                         cond_resched();
> > > >                 }
> > > > 
> > > Sorry, I don't understand what you mean.
> > > 
> > > We don't care what's done in the loop (chip->dev_ready != NULL or, not). We only
> > > check when the loop exit, that READY bit is set in the status.
> > 
> > Well, the logic is suspicious.
> > 
> > 1. For NAND with chip->dev_ready != NULL, why NAND_STATUS_READY should
> > be set? We do not check for this in the loop.
> > 
> > 2. For NAND with chip->dev_ready != NULL, if NAND_STATUS_READY has to be
> > set at the end, why wouldn't we drop this chip_ready part completely? We
> > could just loop while NAND_STATUS_READY is not set.
> > 
> > Isn't this strange?
> 
> Not really. There are 2 methods to wait for an erase/program command completion:
> 
> 1. Wait until nand RnB pin goes high (that's what chip->dev_ready usually does)
> 2. Poll the device: send a status (0x70) command and read status byte in a loop
>    until bit NAND_STATUS_READY is set
> 
> In all cases, you should send a status command after completion, to check if
> the operation was successful. And if the operation completed, the status should
> have bit NAND_STATUS_READY set.
> 
> Method 1 is optimal, you can often use an interrupt to signal completion, and
> no cpu cycles are lost in a polling loop. But flaky hardware (bad pull-ups) or
> bad gpio configuration can make it unreliable.
> 
> Method 2 is the safest, it always works, but it is less efficient.

Thanks, this is basically the reply I waited for - the explanation how
it works. You guys should not assume that I know everything - I just do
my best to keep MTD subsystem working and make people improve it :-) And
sometimes I just have no time to dig things and ask people to educate
me, to save my time :-)

> Here, Matthieu wants to detect cases where method 1 is unreliable or a timeout
> occurs (e.g. somebody forgot to put a nand device on the board :).
> Both conditions are not expected on working hardware.

Fair enough, thanks!

However, the code is not easy to follow, and this assumption which you
explained - "if dev_ready() returns truth, the status command has to be
sent anyway and the READY bit has to be there anyway" - it is subtle,
and makes the nand_base.c less readable, and more error prone.

And there are tons of things like this. And what the community do in
these cases - it forces people who just want to do a simple thing to
also do general clean-ups, at least some reasonable amount of it.

E.g., recently people cleaned-up the partitions stuff, and this started
with our refusal to take a simple path which touched the MTD_PARTITIONS
macro.

So, what I suggested to Matthieu, although in a vague way, is to look
how this subtle dev_ready things could be cleaned-up.

I said that we probably may:

1. Introduce something like default_dev_ready() which falls-back to the
status polling unless the driver provides it's own.
2. Look to all the if (chip->dev_ready) do A else do B things and try to
remove them.

To put it differently, I am trying to encourage you guys to clean-up the
code a bit in this dev_ready/status area before changing this area.

Any ideas? :-)

-- 
Best Regards,
Artem Bityutskiy

[PATCH 3/6] refactor mtd wait code

[Matthieu CASTET]

This patch move the common wait code for read and reset in the function
nand_wait_reset and nand_wait_read.

The only fonctionnal change are :
- there is a 5s timeout for nand reset (there was no timeout before)
- For AUTOINC nand, we wait 100ns before pooling RnB (to be similar of
what we do after a read command).

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
---
 drivers/mtd/nand/nand_base.c |  148 ++++++++++++++++++++++--------------------
 1 files changed, 78 insertions(+), 70 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 095dfea..70af1c0 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -99,6 +99,9 @@ static int nand_get_device(struct nand_chip *chip, struct mtd_info *mtd,
 static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 			     struct mtd_oob_ops *ops);
 
+static void nand_wait_reset(struct mtd_info *mtd, struct nand_chip *chip);
+static void nand_wait_read(struct mtd_info *mtd, struct nand_chip *chip);
+
 /*
  * For devices which display every fart in the system on a separate LED. Is
  * compiled away when LED support is disabled.
@@ -603,33 +606,11 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 		return;
 
 	case NAND_CMD_RESET:
-		if (chip->dev_ready)
-			break;
-		udelay(chip->chip_delay);
-		chip->cmd_ctrl(mtd, NAND_CMD_STATUS,
-			       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))
-				;
+		nand_wait_reset(mtd, chip);
 		return;
-
-		/* This applies to read commands */
-	default:
-		/*
-		 * If we don't have access to the busy pin, we apply the given
-		 * command delay
-		 */
-		if (!chip->dev_ready) {
-			udelay(chip->chip_delay);
-			return;
-		}
 	}
-	/* Apply this short delay always to ensure that we do wait tWB in
-	 * any case on any machine. */
-	ndelay(100);
-
-	nand_wait_ready(mtd);
+	/* This applies to read commands */
+	nand_wait_read(mtd, chip);
 }
 
 /**
@@ -710,15 +691,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 		return;
 
 	case NAND_CMD_RESET:
-		if (chip->dev_ready)
-			break;
-		udelay(chip->chip_delay);
-		chip->cmd_ctrl(mtd, NAND_CMD_STATUS,
-			       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))
-				;
+		nand_wait_reset(mtd, chip);
 		return;
 
 	case NAND_CMD_RNDOUT:
@@ -734,24 +707,9 @@ 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);
-
-		/* This applies to read commands */
-	default:
-		/*
-		 * If we don't have access to the busy pin, we apply the given
-		 * command delay
-		 */
-		if (!chip->dev_ready) {
-			udelay(chip->chip_delay);
-			return;
-		}
 	}
-
-	/* Apply this short delay always to ensure that we do wait tWB in
-	 * any case on any machine. */
-	ndelay(100);
-
-	nand_wait_ready(mtd);
+	/* This applies to read commands */
+	nand_wait_read(mtd, chip);
 }
 
 /**
@@ -838,25 +796,19 @@ static void panic_nand_wait(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 /**
- * nand_wait - [DEFAULT]  wait until the command is done
+ * nand_wait_timeout - wait until the command is done
  * @mtd:	MTD device structure
  * @chip:	NAND chip structure
+ * @timeout : timeout (in jiffies) to wait
  *
- * Wait for command done. This applies to erase and program only
- * Erase can take up to 400ms and program up to 20ms according to
- * general NAND and SmartMedia specs
+ * Wait for command done. This applies to erase, program and reset
  */
-static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
+static int nand_wait_timeout(struct mtd_info *mtd, struct nand_chip *chip,
+					unsigned long timeo)
 {
 
-	unsigned long timeo = jiffies;
 	int status, state = chip->state;
 
-	if (state == FL_ERASING)
-		timeo += (HZ * 400) / 1000;
-	else
-		timeo += (HZ * 20) / 1000;
-
 	led_trigger_event(nand_led_trigger, LED_FULL);
 
 	/* Apply this short delay always to ensure that we do wait tWB in
@@ -891,6 +843,68 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 }
 
 /**
+ * nand_wait - [DEFAULT]  wait until the command is done
+ * @mtd:	MTD device structure
+ * @chip:	NAND chip structure
+ *
+ * Wait for command done. This applies to erase and program only
+ * Erase can take up to 400ms and program up to 20ms according to
+ * general NAND and SmartMedia specs
+ */
+static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
+{
+	unsigned long timeo = jiffies;
+	int state = chip->state;
+
+	if (state == FL_ERASING)
+		timeo += (HZ * 400) / 1000;
+	else
+		timeo += (HZ * 20) / 1000;
+
+	return nand_wait_timeout(mtd, chip, timeo);
+}
+
+/**
+ * nand_wait_reset - wait until the reset is done
+ * @mtd:	MTD device structure
+ * @chip:	NAND chip structure
+ *
+ * Wait for command done. This applies to erase and program only
+ */
+static void nand_wait_reset(struct mtd_info *mtd, struct nand_chip *chip)
+{
+	/* 5s timeout */
+	unsigned long timeo = jiffies + (HZ * 5000) / 1000;
+	nand_wait_timeout(mtd, chip, timeo);
+
+	return;
+}
+
+/**
+  * This is call after sending a read command, or for autoincrement
+  * chip that need it (!NAND_NO_READRDY).
+  *
+  * We can't call NAND_CMD_STATUS here, because the read command
+  * is not finished
+  */
+static void nand_wait_read(struct mtd_info *mtd, struct nand_chip *chip)
+{
+	/*
+	 * If we don't have access to the busy pin, we apply the given
+	 * command delay
+	 */
+	if (!chip->dev_ready) {
+		udelay(chip->chip_delay);
+	}
+	else {
+		/* Apply this short delay always to ensure that we do wait tWB in
+		 * any case on any machine. */
+		ndelay(100);
+		nand_wait_ready(mtd);
+	}
+}
+
+/**
  * __nand_unlock - [REPLACEABLE] unlocks specified locked blocks
  *
  * @mtd: mtd info
@@ -1526,10 +1540,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				 * increment is marked as NOAUTOINCR by the
 				 * board driver.
 				 */
-				if (!chip->dev_ready)
-					udelay(chip->chip_delay);
-				else
-					nand_wait_ready(mtd);
+				nand_wait_read(mtd, chip);
 			}
 		} else {
 			memcpy(buf, chip->buffers->databuf + col, bytes);
@@ -1811,10 +1822,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 			 * chip which does auto increment is marked as
 			 * NOAUTOINCR by the board driver.
 			 */
-			if (!chip->dev_ready)
-				udelay(chip->chip_delay);
-			else
-				nand_wait_ready(mtd);
+			nand_wait_read(mtd, chip);
 		}
 
 		readlen -= len;
-- 
1.7.5.4

Re: [PATCH 3/6] refactor mtd wait code

[Artem Bityutskiy]

On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> +/**
> +  * This is call after sending a read command, or for autoincrement
> +  * chip that need it (!NAND_NO_READRDY).
> +  *
> +  * We can't call NAND_CMD_STATUS here, because the read command
> +  * is not finished
> +  */
> +static void nand_wait_read(struct mtd_info *mtd, struct nand_chip *chip)
> +{
> +	/*
> +	 * If we don't have access to the busy pin, we apply the given
> +	 * command delay
> +	 */
> +	if (!chip->dev_ready) {
> +		udelay(chip->chip_delay);
> +	}
> +	else {
> +		/* Apply this short delay always to ensure that we do wait tWB in
> +		 * any case on any machine. */
> +		ndelay(100);

Please, all these hard-coded numbers should be hidden in the specific
driver.

> +		nand_wait_ready(mtd);
> +	}
> +}
> +
> +/**
>   * __nand_unlock - [REPLACEABLE] unlocks specified locked blocks
>   *
>   * @mtd: mtd info
> @@ -1526,10 +1540,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  				 * increment is marked as NOAUTOINCR by the
>  				 * board driver.
>  				 */
> -				if (!chip->dev_ready)
> -					udelay(chip->chip_delay);
> -				else
> -					nand_wait_ready(mtd);
> +				nand_wait_read(mtd, chip);
>  			}
>  		} else {
>  			memcpy(buf, chip->buffers->databuf + col, bytes);
> @@ -1811,10 +1822,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
>  			 * chip which does auto increment is marked as
>  			 * NOAUTOINCR by the board driver.
>  			 */
> -			if (!chip->dev_ready)
> -				udelay(chip->chip_delay);
> -			else
> -				nand_wait_ready(mtd);
> +			nand_wait_read(mtd, chip);

I think one thing which you should do as part of this series is to get
rid of all the if (!chip->dev_ready) checks. Instead, you should
introduce something like default_chip_ready and make it do the job.
Also, make sure nand_wait_ready() works in that case.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 3/6] refactor mtd wait code

[Artem Bityutskiy]

On Tue, 2011-06-28 at 11:00 +0300, Artem Bityutskiy wrote:
> On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> > +/**
> > +  * This is call after sending a read command, or for autoincrement
> > +  * chip that need it (!NAND_NO_READRDY).
> > +  *
> > +  * We can't call NAND_CMD_STATUS here, because the read command
> > +  * is not finished
> > +  */
> > +static void nand_wait_read(struct mtd_info *mtd, struct nand_chip *chip)
> > +{
> > +	/*
> > +	 * If we don't have access to the busy pin, we apply the given
> > +	 * command delay
> > +	 */
> > +	if (!chip->dev_ready) {
> > +		udelay(chip->chip_delay);
> > +	}
> > +	else {
> > +		/* Apply this short delay always to ensure that we do wait tWB in
> > +		 * any case on any machine. */
> > +		ndelay(100);
> 
> Please, all these hard-coded numbers should be hidden in the specific
> driver.

Or could you please explain a bit better why this delay has to be part
of nand core? And why it is 100 and not 200?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 3/6] refactor mtd wait code

[Matthieu CASTET]

Artem Bityutskiy a écrit :
> On Tue, 2011-06-28 at 11:00 +0300, Artem Bityutskiy wrote:
>> On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
>>> +/**
>>> +  * This is call after sending a read command, or for autoincrement
>>> +  * chip that need it (!NAND_NO_READRDY).
>>> +  *
>>> +  * We can't call NAND_CMD_STATUS here, because the read command
>>> +  * is not finished
>>> +  */
>>> +static void nand_wait_read(struct mtd_info *mtd, struct nand_chip *chip)
>>> +{
>>> +	/*
>>> +	 * If we don't have access to the busy pin, we apply the given
>>> +	 * command delay
>>> +	 */
>>> +	if (!chip->dev_ready) {
>>> +		udelay(chip->chip_delay);
>>> +	}
>>> +	else {
>>> +		/* Apply this short delay always to ensure that we do wait tWB in
>>> +		 * any case on any machine. */
>>> +		ndelay(100);
>> Please, all these hard-coded numbers should be hidden in the specific
>> driver.
> 
> Or could you please explain a bit better why this delay has to be part
> of nand core? And why it is 100 and not 200?
> 
This delay is already in the nand core. I only put it in a common function :

> -	/* Apply this short delay always to ensure that we do wait tWB in
> -	 * any case on any machine. */
> -	ndelay(100);
> -
> -	nand_wait_ready(mtd);
> +	/* This applies to read commands */
> +	nand_wait_read(mtd, chip);
>  }

> -
> -	/* Apply this short delay always to ensure that we do wait tWB in
> -	 * any case on any machine. */
> -	ndelay(100);
> -
> -	nand_wait_ready(mtd);
> +	/* This applies to read commands */
> +	nand_wait_read(mtd, chip);
>  }


And I think it is 100 ns, because it is the worst case for tWB.


Matthieu

Re: [PATCH 3/6] refactor mtd wait code

[Artem Bityutskiy]

On Tue, 2011-06-28 at 17:00 +0200, Matthieu CASTET wrote:
> Artem Bityutskiy a écrit :
> > On Tue, 2011-06-28 at 11:00 +0300, Artem Bityutskiy wrote:
> >> On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> >>> +/**
> >>> +  * This is call after sending a read command, or for autoincrement
> >>> +  * chip that need it (!NAND_NO_READRDY).
> >>> +  *
> >>> +  * We can't call NAND_CMD_STATUS here, because the read command
> >>> +  * is not finished
> >>> +  */
> >>> +static void nand_wait_read(struct mtd_info *mtd, struct nand_chip *chip)
> >>> +{
> >>> +	/*
> >>> +	 * If we don't have access to the busy pin, we apply the given
> >>> +	 * command delay
> >>> +	 */
> >>> +	if (!chip->dev_ready) {
> >>> +		udelay(chip->chip_delay);
> >>> +	}
> >>> +	else {
> >>> +		/* Apply this short delay always to ensure that we do wait tWB in
> >>> +		 * any case on any machine. */
> >>> +		ndelay(100);
> >> Please, all these hard-coded numbers should be hidden in the specific
> >> driver.
> > 
> > Or could you please explain a bit better why this delay has to be part
> > of nand core? And why it is 100 and not 200?
> > 
> This delay is already in the nand core. I only put it in a common function :

OK, sorry, but would it be possible to take a look at how this patch
could be split on smaller ones to make review a bit simpler?

-- 
Best Regards,
Artem Bityutskiy

[PATCH 4/6] nand_wait_read : add code to wait on status for LP

[Matthieu CASTET]

Use read status instead of waiting a fixed time.

For reading data after read status a command 0x0 should be issued (on large page) [1].
Only apply it for driver that use nand_command_lp, because other cmdfunc won't expect
NAND_CMD_READ0 without address.

[1]
While monitoring the read status to determine when the tR (transfer from Flash array to page
register) is complete, the host shall re-issue a command value of 00h to start reading data.
Issuing a command value of 00h will cause data to be returned starting at the selected column
address.

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
---
 drivers/mtd/nand/nand_base.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 70af1c0..5f0a0de 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -703,6 +703,8 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 		return;
 
 	case NAND_CMD_READ0:
+		if (column == -1 && page_addr == -1)
+			return;
 		chip->cmd_ctrl(mtd, NAND_CMD_READSTART,
 			       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
 		chip->cmd_ctrl(mtd, NAND_CMD_NONE,
@@ -889,11 +891,20 @@ static void nand_wait_reset(struct mtd_info *mtd, struct nand_chip *chip)
   */
 static void nand_wait_read(struct mtd_info *mtd, struct nand_chip *chip)
 {
+	if (!chip->dev_ready && chip->cmdfunc == nand_command_lp) {
+		int status;
+		/* same timeout than nand_wait_ready */
+		unsigned long timeo = jiffies + (2 * HZ) / 1000;
+		status = nand_wait_timeout(mtd, chip, timeo);
+		WARN_ON(!(status & NAND_STATUS_READY));
+		/* start reading data */
+		chip->cmdfunc(mtd, NAND_CMD_READ0, -1, -1);
+	}
 	/*
 	 * If we don't have access to the busy pin, we apply the given
 	 * command delay
 	 */
-	if (!chip->dev_ready) {
+	else if (!chip->dev_ready) {
 		udelay(chip->chip_delay);
 	}
 	else {
-- 
1.7.5.4

[PATCH 5/6] nand_flash_detect_onfi propagate busw info

[Matthieu CASTET]

there is a bug in nand_flash_detect_onfi, busw need to be passed
by pointer to return it.

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
---
 drivers/mtd/nand/nand_base.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 5f0a0de..e7c8bdd 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2850,7 +2850,7 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise
  */
 static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
-					int busw)
+					int *busw)
 {
 	struct nand_onfi_params *p = &chip->onfi_params;
 	int i;
@@ -2905,9 +2905,9 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	mtd->erasesize = le32_to_cpu(p->pages_per_block) * mtd->writesize;
 	mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
 	chip->chipsize = (uint64_t)le32_to_cpu(p->blocks_per_lun) * mtd->erasesize;
-	busw = 0;
+	*busw = 0;
 	if (le16_to_cpu(p->features) & 1)
-		busw = NAND_BUSWIDTH_16;
+		*busw = NAND_BUSWIDTH_16;
 
 	chip->options &= ~NAND_CHIPOPTIONS_MSK;
 	chip->options |= (NAND_NO_READRDY |
@@ -2973,7 +2973,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	chip->onfi_version = 0;
 	if (!type->name || !type->pagesize) {
 		/* Check is chip is ONFI compliant */
-		ret = nand_flash_detect_onfi(mtd, chip, busw);
+		ret = nand_flash_detect_onfi(mtd, chip, &busw);
 		if (ret)
 			goto ident_done;
 	}
-- 
1.7.5.4

Re: [PATCH 5/6] nand_flash_detect_onfi propagate busw info

[Brian Norris]

On Sun, Jun 26, 2011 at 9:26 AM, Matthieu CASTET
<matthieu.castet@parrot.com> wrote:
> there is a bug in nand_flash_detect_onfi, busw need to be passed
> by pointer to return it.
>
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> ---
>  drivers/mtd/nand/nand_base.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)

Acked-by: Brian Norris <computersforpeace@gmail.com>

Re: [PATCH 5/6] nand_flash_detect_onfi propagate busw info

[Artem Bityutskiy]

On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> there is a bug in nand_flash_detect_onfi, busw need to be passed
> by pointer to return it.
> 
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> ---
>  drivers/mtd/nand/nand_base.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)

Pushed this patch to the l2 tree. It did not apply cleanly, though,
would be nice if your patches were based on l2-mtd-2.6.git.

-- 
Best Regards,
Artem Bityutskiy

[PATCH 6/6] add NAND_BUSWIDTH_AUTO

[Matthieu CASTET]

Nand read id and Read Parameter (onfi) return data on low 8 bits.
So if the driver start in 8 bit mode, it is possible to detect nand
buswidth, and configure the controller to the correct buswidth
after nand_scan_ident.

This is a new flag, so only driver that know what there are doing
will use it.

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
---
 drivers/mtd/nand/nand_base.c |    7 ++++++-
 include/linux/mtd/nand.h     |    5 +++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index e7c8bdd..34f441e 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3096,11 +3096,16 @@ ident_done:
 			break;
 	}
 
+	if (chip->options & NAND_BUSWIDTH_AUTO) {
+		BUG_ON(chip->options & NAND_BUSWIDTH_16);
+		chip->options |= busw;
+		nand_set_defaults(chip, busw);
+	}
 	/*
 	 * Check, if buswidth is correct. Hardware drivers should set
 	 * chip correct !
 	 */
-	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
+	else if (busw != (chip->options & NAND_BUSWIDTH_16)) {
 		printk(KERN_INFO "NAND device: Manufacturer ID:"
 		       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
 		       *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c2b9ac4..cdda28a 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -240,6 +240,11 @@ typedef enum {
 #define NAND_USE_FLASH_BBT_NO_OOB	0x00800000
 /* Create an empty BBT with no vendor information if the BBT is available */
 #define NAND_CREATE_EMPTY_BBT		0x01000000
+/* autodetect nand buswidth with readid. This suppose the driver will
+ * configure the hardware has 8 bit in nand_scan_ident, and update
+ * configuration before calling nand_scan_tail
+ */
+#define NAND_BUSWIDTH_AUTO			0x02000000
 
 /* Options set by nand scan */
 /* Nand scan has allocated controller struct */
-- 
1.7.5.4

Re: [PATCH 6/6] add NAND_BUSWIDTH_AUTO

[Brian Norris]

Hi,

I have a few comments below.

On Sun, Jun 26, 2011 at 9:26 AM, Matthieu CASTET
<matthieu.castet@parrot.com> wrote:
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3096,11 +3096,16 @@ ident_done:
...
> +       if (chip->options & NAND_BUSWIDTH_AUTO) {
> +               BUG_ON(chip->options & NAND_BUSWIDTH_16);
> +               chip->options |= busw;
> +               nand_set_defaults(chip, busw);
> +       }

Doesn't this get called after nand_set_defaults() is already called?
(see nand_scan_ident) If so, then changing the buswidth here won't
actually affect any of the function assignments. That's probably not
your expected behavior, is it?

> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -240,6 +240,11 @@ typedef enum {
...
> +/* autodetect nand buswidth with readid. This suppose the driver will
> + * configure the hardware has 8 bit in nand_scan_ident, and update
> + * configuration before calling nand_scan_tail
> + */

This is not the correct multi-line comment format.

> +#define NAND_BUSWIDTH_AUTO                     0x02000000

I recently did some revamping of a lot of these flags, so the
numbering has all changed in Artem's staging tree, l2-mtd-2.6.git.
Assuming both our patches are taken into mainline, it'd probably be
good if you rebased on the l2 tree and choose a new bit value for this
flag. AFAICT it'd be good like:

+#define NAND_BUSWIDTH_AUTO                     0x00080000

Brian

Re: [PATCH 1/6] nand_wait_ready timeout fix

[Artem Bityutskiy]

On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> nand_wait_ready timeout should not assume HZ=1000.
> Make it independent of HZ value.
> 
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> ---
>  drivers/mtd/nand/nand_base.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index a46e9bb..a3c7fd3 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -512,7 +512,7 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
>  void nand_wait_ready(struct mtd_info *mtd)
>  {
>  	struct nand_chip *chip = mtd->priv;
> -	unsigned long timeo = jiffies + 2;
> +	unsigned long timeo = jiffies + (2 * HZ) / 1000;

I agree that the code is buggy, but your fix is strange: if HZ = 100, (2
* HZ) / 1000 will be zero?

I think you should instead know for how many msecs you want to wait, and
use msecs_to_jiffies().

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/6] nand_wait_ready timeout fix

[Matthieu CASTET]

Artem Bityutskiy a écrit :
> On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
>> nand_wait_ready timeout should not assume HZ=1000.
>> Make it independent of HZ value.
>>
>> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
>> ---
>>  drivers/mtd/nand/nand_base.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index a46e9bb..a3c7fd3 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -512,7 +512,7 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
>>  void nand_wait_ready(struct mtd_info *mtd)
>>  {
>>  	struct nand_chip *chip = mtd->priv;
>> -	unsigned long timeo = jiffies + 2;
>> +	unsigned long timeo = jiffies + (2 * HZ) / 1000;
> 
> I agree that the code is buggy, but your fix is strange: if HZ = 100, (2
> * HZ) / 1000 will be zero?
> 
> I think you should instead know for how many msecs you want to wait, and
> use msecs_to_jiffies().
> 
Your right, I assume the code was written for HZ=100, and the code should wait
for 20 ms :
unsigned long timeo = jiffies + (20 * HZ) / 1000;

this will match with the
static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
{

    unsigned long timeo = jiffies;
    int status, state = chip->state;

    if (state == FL_ERASING)
        timeo += (HZ * 400) / 1000;
    else
        timeo += (HZ * 20) / 1000;
[...]
}

Matthieu

Re: [PATCH 1/6] nand_wait_ready timeout fix

[Artem Bityutskiy]

On Tue, 2011-06-28 at 17:09 +0200, Matthieu CASTET wrote:
> >>  	struct nand_chip *chip = mtd->priv;
> >> -	unsigned long timeo = jiffies + 2;
> >> +	unsigned long timeo = jiffies + (2 * HZ) / 1000;
> > 
> > I agree that the code is buggy, but your fix is strange: if HZ = 100, (2
> > * HZ) / 1000 will be zero?
> > 
> > I think you should instead know for how many msecs you want to wait, and
> > use msecs_to_jiffies().
> > 
> Your right, I assume the code was written for HZ=100, and the code should wait
> for 20 ms :
> unsigned long timeo = jiffies + (20 * HZ) / 1000;
> 
> this will match with the
> static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> {
> 
>     unsigned long timeo = jiffies;
>     int status, state = chip->state;
> 
>     if (state == FL_ERASING)
>         timeo += (HZ * 400) / 1000;
>     else
>         timeo += (HZ * 20) / 1000;
> [...]
> }

I think we should use msecs_to_jiffies() in nand_wait() as well. Why
relying on weird calculations with HZ?

-- 
Best Regards,
Artem Bityutskiy