* [PATCH 1/6] nand_wait_ready timeout fix
@ 2011-06-26 16:26 Matthieu CASTET
2011-06-26 16:26 ` [PATCH 2/6] nand_wait : warn if the nand is busy on exit Matthieu CASTET
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-26 16:26 UTC (permalink / raw)
To: linux-mtd; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/6] nand_wait : warn if the nand is busy on exit
2011-06-26 16:26 [PATCH 1/6] nand_wait_ready timeout fix Matthieu CASTET
@ 2011-06-26 16:26 ` Matthieu CASTET
2011-06-28 7:57 ` Artem Bityutskiy
2011-06-26 16:26 ` [PATCH 3/6] refactor mtd wait code Matthieu CASTET
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-26 16:26 UTC (permalink / raw)
To: linux-mtd; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/6] refactor mtd wait code
2011-06-26 16:26 [PATCH 1/6] nand_wait_ready timeout fix Matthieu CASTET
2011-06-26 16:26 ` [PATCH 2/6] nand_wait : warn if the nand is busy on exit Matthieu CASTET
@ 2011-06-26 16:26 ` Matthieu CASTET
2011-06-28 8:00 ` Artem Bityutskiy
2011-06-26 16:26 ` [PATCH 4/6] nand_wait_read : add code to wait on status for LP Matthieu CASTET
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-26 16:26 UTC (permalink / raw)
To: linux-mtd; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/6] nand_wait_read : add code to wait on status for LP
2011-06-26 16:26 [PATCH 1/6] nand_wait_ready timeout fix Matthieu CASTET
2011-06-26 16:26 ` [PATCH 2/6] nand_wait : warn if the nand is busy on exit Matthieu CASTET
2011-06-26 16:26 ` [PATCH 3/6] refactor mtd wait code Matthieu CASTET
@ 2011-06-26 16:26 ` Matthieu CASTET
2011-06-26 16:26 ` [PATCH 5/6] nand_flash_detect_onfi propagate busw info Matthieu CASTET
` (2 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-26 16:26 UTC (permalink / raw)
To: linux-mtd; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/6] nand_flash_detect_onfi propagate busw info
2011-06-26 16:26 [PATCH 1/6] nand_wait_ready timeout fix Matthieu CASTET
` (2 preceding siblings ...)
2011-06-26 16:26 ` [PATCH 4/6] nand_wait_read : add code to wait on status for LP Matthieu CASTET
@ 2011-06-26 16:26 ` Matthieu CASTET
2011-06-29 16:38 ` Brian Norris
2011-06-30 11:47 ` Artem Bityutskiy
2011-06-26 16:26 ` [PATCH 6/6] add NAND_BUSWIDTH_AUTO Matthieu CASTET
2011-06-28 7:48 ` [PATCH 1/6] nand_wait_ready timeout fix Artem Bityutskiy
5 siblings, 2 replies; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-26 16:26 UTC (permalink / raw)
To: linux-mtd; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 6/6] add NAND_BUSWIDTH_AUTO
2011-06-26 16:26 [PATCH 1/6] nand_wait_ready timeout fix Matthieu CASTET
` (3 preceding siblings ...)
2011-06-26 16:26 ` [PATCH 5/6] nand_flash_detect_onfi propagate busw info Matthieu CASTET
@ 2011-06-26 16:26 ` Matthieu CASTET
2011-06-29 16:37 ` Brian Norris
2011-06-28 7:48 ` [PATCH 1/6] nand_wait_ready timeout fix Artem Bityutskiy
5 siblings, 1 reply; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-26 16:26 UTC (permalink / raw)
To: linux-mtd; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] nand_wait_ready timeout fix
2011-06-26 16:26 [PATCH 1/6] nand_wait_ready timeout fix Matthieu CASTET
` (4 preceding siblings ...)
2011-06-26 16:26 ` [PATCH 6/6] add NAND_BUSWIDTH_AUTO Matthieu CASTET
@ 2011-06-28 7:48 ` Artem Bityutskiy
2011-06-28 15:09 ` Matthieu CASTET
5 siblings, 1 reply; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-28 7:48 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit
2011-06-26 16:26 ` [PATCH 2/6] nand_wait : warn if the nand is busy on exit Matthieu CASTET
@ 2011-06-28 7:57 ` Artem Bityutskiy
2011-06-28 15:03 ` Matthieu CASTET
0 siblings, 1 reply; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-28 7:57 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/6] refactor mtd wait code
2011-06-26 16:26 ` [PATCH 3/6] refactor mtd wait code Matthieu CASTET
@ 2011-06-28 8:00 ` Artem Bityutskiy
2011-06-28 8:03 ` Artem Bityutskiy
0 siblings, 1 reply; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-28 8:00 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/6] refactor mtd wait code
2011-06-28 8:00 ` Artem Bityutskiy
@ 2011-06-28 8:03 ` Artem Bityutskiy
2011-06-28 15:00 ` Matthieu CASTET
0 siblings, 1 reply; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-28 8:03 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/6] refactor mtd wait code
2011-06-28 8:03 ` Artem Bityutskiy
@ 2011-06-28 15:00 ` Matthieu CASTET
2011-06-29 6:09 ` Artem Bityutskiy
0 siblings, 1 reply; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-28 15:00 UTC (permalink / raw)
To: dedekind1; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit
2011-06-28 7:57 ` Artem Bityutskiy
@ 2011-06-28 15:03 ` Matthieu CASTET
2011-06-28 19:05 ` Artem Bityutskiy
0 siblings, 1 reply; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-28 15:03 UTC (permalink / raw)
To: dedekind1; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] nand_wait_ready timeout fix
2011-06-28 7:48 ` [PATCH 1/6] nand_wait_ready timeout fix Artem Bityutskiy
@ 2011-06-28 15:09 ` Matthieu CASTET
2011-06-29 6:08 ` Artem Bityutskiy
0 siblings, 1 reply; 21+ messages in thread
From: Matthieu CASTET @ 2011-06-28 15:09 UTC (permalink / raw)
To: dedekind1; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit
2011-06-28 15:03 ` Matthieu CASTET
@ 2011-06-28 19:05 ` Artem Bityutskiy
2011-06-29 13:59 ` Ivan Djelic
0 siblings, 1 reply; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-28 19:05 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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 (Битюцкий Артём)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] nand_wait_ready timeout fix
2011-06-28 15:09 ` Matthieu CASTET
@ 2011-06-29 6:08 ` Artem Bityutskiy
0 siblings, 0 replies; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-29 6:08 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/6] refactor mtd wait code
2011-06-28 15:00 ` Matthieu CASTET
@ 2011-06-29 6:09 ` Artem Bityutskiy
0 siblings, 0 replies; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-29 6:09 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit
2011-06-28 19:05 ` Artem Bityutskiy
@ 2011-06-29 13:59 ` Ivan Djelic
2011-06-30 12:36 ` Artem Bityutskiy
0 siblings, 1 reply; 21+ messages in thread
From: Ivan Djelic @ 2011-06-29 13:59 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: linux-mtd, Matthieu Castet
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 6/6] add NAND_BUSWIDTH_AUTO
2011-06-26 16:26 ` [PATCH 6/6] add NAND_BUSWIDTH_AUTO Matthieu CASTET
@ 2011-06-29 16:37 ` Brian Norris
0 siblings, 0 replies; 21+ messages in thread
From: Brian Norris @ 2011-06-29 16:37 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] nand_flash_detect_onfi propagate busw info
2011-06-26 16:26 ` [PATCH 5/6] nand_flash_detect_onfi propagate busw info Matthieu CASTET
@ 2011-06-29 16:38 ` Brian Norris
2011-06-30 11:47 ` Artem Bityutskiy
1 sibling, 0 replies; 21+ messages in thread
From: Brian Norris @ 2011-06-29 16:38 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] nand_flash_detect_onfi propagate busw info
2011-06-26 16:26 ` [PATCH 5/6] nand_flash_detect_onfi propagate busw info Matthieu CASTET
2011-06-29 16:38 ` Brian Norris
@ 2011-06-30 11:47 ` Artem Bityutskiy
1 sibling, 0 replies; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-30 11:47 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-mtd
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit
2011-06-29 13:59 ` Ivan Djelic
@ 2011-06-30 12:36 ` Artem Bityutskiy
0 siblings, 0 replies; 21+ messages in thread
From: Artem Bityutskiy @ 2011-06-30 12:36 UTC (permalink / raw)
To: Ivan Djelic; +Cc: linux-mtd, Matthieu Castet
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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2011-06-30 12:36 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-26 16:26 [PATCH 1/6] nand_wait_ready timeout fix Matthieu CASTET
2011-06-26 16:26 ` [PATCH 2/6] nand_wait : warn if the nand is busy on exit Matthieu CASTET
2011-06-28 7:57 ` Artem Bityutskiy
2011-06-28 15:03 ` Matthieu CASTET
2011-06-28 19:05 ` Artem Bityutskiy
2011-06-29 13:59 ` Ivan Djelic
2011-06-30 12:36 ` Artem Bityutskiy
2011-06-26 16:26 ` [PATCH 3/6] refactor mtd wait code Matthieu CASTET
2011-06-28 8:00 ` Artem Bityutskiy
2011-06-28 8:03 ` Artem Bityutskiy
2011-06-28 15:00 ` Matthieu CASTET
2011-06-29 6:09 ` Artem Bityutskiy
2011-06-26 16:26 ` [PATCH 4/6] nand_wait_read : add code to wait on status for LP Matthieu CASTET
2011-06-26 16:26 ` [PATCH 5/6] nand_flash_detect_onfi propagate busw info Matthieu CASTET
2011-06-29 16:38 ` Brian Norris
2011-06-30 11:47 ` Artem Bityutskiy
2011-06-26 16:26 ` [PATCH 6/6] add NAND_BUSWIDTH_AUTO Matthieu CASTET
2011-06-29 16:37 ` Brian Norris
2011-06-28 7:48 ` [PATCH 1/6] nand_wait_ready timeout fix Artem Bityutskiy
2011-06-28 15:09 ` Matthieu CASTET
2011-06-29 6:08 ` Artem Bityutskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.