[PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Masahiro Yamada]

1/2 : add NAND_WAIT_TWHR and nand_whr_delay().
      You can set this new flag if you want nand_command(_lp)
      to insert tWHR delay where needed.

2/2 : Fix Denali setup_data_interface.
      Boris' suggestion in v1 was a good reminder that
      made me realize tCCS was missing in the driver.  Fix it now.


Changes in v2:
  - Add nand_whr_delay() helper
    Wait for tWHR only for drivers that explicitly set NAND_WAIT_TWHR flag
  - newly added

Masahiro Yamada (2):
  mtd: nand: wait for tWHR after NAND_CMD_STATUS / NAND_CMD_READID
  mtd: nand: denali: fix setup_data_interface to meet tCCS delay

 drivers/mtd/nand/denali.c    | 10 ++++++++--
 drivers/mtd/nand/nand_base.c | 21 +++++++++++++++++++--
 include/linux/mtd/rawnand.h  | 13 ++++++++-----
 3 files changed, 35 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] mtd: nand: wait for tWHR after NAND_CMD_STATUS / NAND_CMD_READID

[Masahiro Yamada]

For commands such as Read Status, Read ID, etc. the controller needs
to wait for tWHR before it reads out the first data.  If the controller
does not take care of it, the driver has to wait explicitly to make
sure to meet the spec.

Introduce NAND_WAIT_TWHR, which works like NAND_WAIT_TCCS.  The
driver can set this flag to let the core to handle the tWHR delay.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Add nand_whr_delay() helper
    Wait for tWHR only for drivers that explicitly set NAND_WAIT_TWHR flag

 drivers/mtd/nand/nand_base.c | 21 +++++++++++++++++++--
 include/linux/mtd/rawnand.h  | 13 ++++++++-----
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b1cf32c..455085d 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -676,6 +676,17 @@ static void nand_wait_status_ready(struct mtd_info *mtd, unsigned long timeo)
 	} while (time_before(jiffies, timeo));
 };
 
+static void nand_whr_delay(struct nand_chip *chip)
+{
+	if (!(chip->options & NAND_WAIT_TWHR))
+		return;
+
+	if (chip->data_interface && chip->data_interface->timings.sdr.tWHR_min)
+		ndelay(chip->data_interface->timings.sdr.tWHR_min / 1000);
+	else
+		ndelay(200);
+}
+
 /**
  * nand_command - [DEFAULT] Send command to NAND device
  * @mtd: MTD device structure
@@ -742,9 +753,12 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 	case NAND_CMD_ERASE1:
 	case NAND_CMD_ERASE2:
 	case NAND_CMD_SEQIN:
+	case NAND_CMD_SET_FEATURES:
+		return;
+
 	case NAND_CMD_STATUS:
 	case NAND_CMD_READID:
-	case NAND_CMD_SET_FEATURES:
+		nand_whr_delay(chip);
 		return;
 
 	case NAND_CMD_RESET:
@@ -871,9 +885,12 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 	case NAND_CMD_ERASE1:
 	case NAND_CMD_ERASE2:
 	case NAND_CMD_SEQIN:
+	case NAND_CMD_SET_FEATURES:
+		return;
+
 	case NAND_CMD_STATUS:
 	case NAND_CMD_READID:
-	case NAND_CMD_SET_FEATURES:
+		nand_whr_delay(chip);
 		return;
 
 	case NAND_CMD_RNDIN:
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 749bb08..bb0165b 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -213,13 +213,16 @@ enum nand_ecc_algo {
 
 /*
  * In case your controller is implementing ->cmd_ctrl() and is relying on the
- * default ->cmdfunc() implementation, you may want to let the core handle the
- * tCCS delay which is required when a column change (RNDIN or RNDOUT) is
- * requested.
- * If your controller already takes care of this delay, you don't need to set
- * this flag.
+ * default ->cmdfunc() implementation, you may want to let the core handle
+ * some delays to meet the timing specification.
+ * If your controller already takes care of these delays, you don't need to set
+ * the following flags.
  */
+
+/* tCCS for a column change (RNDIN or RNDOUT) */
 #define NAND_WAIT_TCCS		0x00200000
+/* tWHR for STATUS, READID, etc. */
+#define NAND_WAIT_TWHR		0x00400000
 
 /* Options set by nand scan */
 /* Nand scan has allocated controller struct */
-- 
2.7.4

[PATCH v2 2/2] mtd: nand: denali: fix setup_data_interface to meet tCCS delay

[Masahiro Yamada]

The WE_2_RE register specifies the number of clock cycles inserted
between the rising edge of #WE and the falling edge of #RE.

The current setup_data_interface implementation takes care of tWHR,
but tCCS is missing.  Wait max(tCSS, tWHR) to meet the spec.

With setup_data_interface() is properly programmed, the Denali NAND
controller can observe the timing, so NAND_WAIT_{TCCS,TWHR} flag is
unneeded.  Say this in the comment block.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - newly added

 drivers/mtd/nand/denali.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 0b268ec..332c022 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1004,8 +1004,14 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 	tmp |= FIELD_PREP(RE_2_RE__VALUE, re_2_re);
 	iowrite32(tmp, denali->reg + RE_2_RE);
 
-	/* tWHR -> WE_2_RE */
-	we_2_re = DIV_ROUND_UP(timings->tWHR_min, t_clk);
+	/*
+	 * tCCS, tWHR -> WE_2_RE
+	 *
+	 * With WE_2_RE properly set, the Denali controller automatically takes
+	 * care of tCCS, tWHR; the driver need not set NAND_WAIT_{TCCS,TWHR}.
+	 */
+	we_2_re = DIV_ROUND_UP(max(timings->tCCS_min, timings->tWHR_min),
+			       t_clk);
 	we_2_re = min_t(int, we_2_re, TWHR2_AND_WE_2_RE__WE_2_RE);
 
 	tmp = ioread32(denali->reg + TWHR2_AND_WE_2_RE);
-- 
2.7.4

Re: [PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Boris Brezillon]

On Fri, 29 Sep 2017 19:38:38 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> 1/2 : add NAND_WAIT_TWHR and nand_whr_delay().
>       You can set this new flag if you want nand_command(_lp)
>       to insert tWHR delay where needed.
> 
> 2/2 : Fix Denali setup_data_interface.
>       Boris' suggestion in v1 was a good reminder that
>       made me realize tCCS was missing in the driver.  Fix it now.
> 
> 
> Changes in v2:
>   - Add nand_whr_delay() helper
>     Wait for tWHR only for drivers that explicitly set NAND_WAIT_TWHR flag
>   - newly added
> 
> Masahiro Yamada (2):
>   mtd: nand: wait for tWHR after NAND_CMD_STATUS / NAND_CMD_READID

Hm, I thought you were introducing this to then use it in the denali
driver. Sorry, but I don't want to apply something that nobody needs.
If someone ever complains about a missing delay I'll point him to your
patch, but until then I'll keep the core unchanged.

>   mtd: nand: denali: fix setup_data_interface to meet tCCS delay

This one is valid. I'll queue it to nand/next soon.

Thanks,

Boris

> 
>  drivers/mtd/nand/denali.c    | 10 ++++++++--
>  drivers/mtd/nand/nand_base.c | 21 +++++++++++++++++++--
>  include/linux/mtd/rawnand.h  | 13 ++++++++-----
>  3 files changed, 35 insertions(+), 9 deletions(-)
> 

Re: [PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Masahiro Yamada]

2017-09-29 21:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Fri, 29 Sep 2017 19:38:38 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> 1/2 : add NAND_WAIT_TWHR and nand_whr_delay().
>>       You can set this new flag if you want nand_command(_lp)
>>       to insert tWHR delay where needed.
>>
>> 2/2 : Fix Denali setup_data_interface.
>>       Boris' suggestion in v1 was a good reminder that
>>       made me realize tCCS was missing in the driver.  Fix it now.
>>
>>
>> Changes in v2:
>>   - Add nand_whr_delay() helper
>>     Wait for tWHR only for drivers that explicitly set NAND_WAIT_TWHR flag
>>   - newly added
>>
>> Masahiro Yamada (2):
>>   mtd: nand: wait for tWHR after NAND_CMD_STATUS / NAND_CMD_READID
>
> Hm, I thought you were introducing this to then use it in the denali
> driver. Sorry, but I don't want to apply something that nobody needs.
> If someone ever complains about a missing delay I'll point him to your
> patch, but until then I'll keep the core unchanged.


At first, I thought this was necessary for me,
but I realized it was my misunderstanding.


Please let me explain one more.

See commit 3158fa0e739615769cc047d2428f30f4c3b6640e.

Prior to that commit, READID waited for #R/B transition,
it was wrong, so I fixed it.

However, it dropped the delay completely.
If somebody was implicitly relying on the delay of chip->dev_ready,
the first byte might be read out before the valid data
is available.

This was motivation of v1, where inserted ndelay(200)
unconditionally.





>>   mtd: nand: denali: fix setup_data_interface to meet tCCS delay
>
> This one is valid. I'll queue it to nand/next soon.

If you drop 1/2, please let me do v3.

V2 mentions NAND_WAIT_TWHR, this is strange.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Boris Brezillon]

On Fri, 29 Sep 2017 23:06:42 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> 2017-09-29 21:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Fri, 29 Sep 2017 19:38:38 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> 1/2 : add NAND_WAIT_TWHR and nand_whr_delay().
> >>       You can set this new flag if you want nand_command(_lp)
> >>       to insert tWHR delay where needed.
> >>
> >> 2/2 : Fix Denali setup_data_interface.
> >>       Boris' suggestion in v1 was a good reminder that
> >>       made me realize tCCS was missing in the driver.  Fix it now.
> >>
> >>
> >> Changes in v2:
> >>   - Add nand_whr_delay() helper
> >>     Wait for tWHR only for drivers that explicitly set NAND_WAIT_TWHR flag
> >>   - newly added
> >>
> >> Masahiro Yamada (2):
> >>   mtd: nand: wait for tWHR after NAND_CMD_STATUS / NAND_CMD_READID  
> >
> > Hm, I thought you were introducing this to then use it in the denali
> > driver. Sorry, but I don't want to apply something that nobody needs.
> > If someone ever complains about a missing delay I'll point him to your
> > patch, but until then I'll keep the core unchanged.  
> 
> 
> At first, I thought this was necessary for me,
> but I realized it was my misunderstanding.
> 
> 
> Please let me explain one more.
> 
> See commit 3158fa0e739615769cc047d2428f30f4c3b6640e.
> 
> Prior to that commit, READID waited for #R/B transition,
> it was wrong, so I fixed it.
> 
> However, it dropped the delay completely.
> If somebody was implicitly relying on the delay of chip->dev_ready,
> the first byte might be read out before the valid data
> is available.
> 
> This was motivation of v1, where inserted ndelay(200)
> unconditionally.

Okay. Anyway, this extra delay is activated with an opt-in flag (I
know, I'm the one who asked that), and noone set this flag in
chip->options, so, if there's a bug, it's still here even after
applying "mtd: nand: wait for tWHR after NAND_CMD_STATUS /
NAND_CMD_READID".

Honestly, I think all advanced controllers have the tWHR/tRHW timings
enforced in the HW logic (configurable through a reg). This leaves
basic controllers like the nand-gpio one, and even for these ones, the
delay between the chip->cmd_ctrl(ADDR) and chip->read_buf() calls is
probably long enough to hide the problem.

Note that I'm absolutely not against this patch, it's just that I'd
like to have a real user before merging this logic.

> 
> 
> 
> 
> 
> >>   mtd: nand: denali: fix setup_data_interface to meet tCCS delay  
> >
> > This one is valid. I'll queue it to nand/next soon.  
> 
> If you drop 1/2, please let me do v3.
> 
> V2 mentions NAND_WAIT_TWHR, this is strange.
> 

Sure.

> 
> 

Re: [PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Masahiro Yamada]

(+CC Marc Gonzalez)

2017-09-29 21:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Fri, 29 Sep 2017 19:38:38 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> 1/2 : add NAND_WAIT_TWHR and nand_whr_delay().
>>       You can set this new flag if you want nand_command(_lp)
>>       to insert tWHR delay where needed.
>>
>> 2/2 : Fix Denali setup_data_interface.
>>       Boris' suggestion in v1 was a good reminder that
>>       made me realize tCCS was missing in the driver.  Fix it now.
>>
>>
>> Changes in v2:
>>   - Add nand_whr_delay() helper
>>     Wait for tWHR only for drivers that explicitly set NAND_WAIT_TWHR flag
>>   - newly added
>>
>> Masahiro Yamada (2):
>>   mtd: nand: wait for tWHR after NAND_CMD_STATUS / NAND_CMD_READID
>
> Hm, I thought you were introducing this to then use it in the denali
> driver. Sorry, but I don't want to apply something that nobody needs.
> If someone ever complains about a missing delay I'll point him to your
> patch, but until then I'll keep the core unchanged.

Perhaps, Marc Gonzalez is the person.


tango_nand.c is the only driver that sets NAND_WAIT_TCCS.

Now, there is completely no delay when reading out the ID.


One safe change might be apply this patch,
then set NAND_WAIT_TWHR to tango_nand.c


I am guessing NAND_WAIT_TCCS was added for it.
Theoretically, I do not see logical difference between tCCS and tWHR.

I am CCing Marc Gonzalez, the author of tango_nand.c




commit 6ea40a3ba93e1b14ffb349e276f9dfefc4334b99
Author: Boris Brezillon <boris.brezillon@free-electrons.com>
Date:   Sat Oct 1 10:24:03 2016 +0200

    mtd: nand: Wait tCCS after a column change

    Drivers implementing ->cmd_ctrl() and relying on the default ->cmdfunc()
    implementation usually don't wait tCCS when a column change (RNDIN or
    RNDOUT) is requested.
    Add an option flag to ask the core to do so (note that we keep this as
    an opt-in to avoid breaking existing implementations), and make use of
    the ->data_interface information is available (otherwise, wait 500ns).

    Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
    Tested-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Marc Gonzalez]

On 29/09/2017 16:33, Masahiro Yamada wrote:

> (+CC Marc Gonzalez)
> 
> 2017-09-29 21:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> On Fri, 29 Sep 2017 19:38:38 +0900
>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>
>>> 1/2 : add NAND_WAIT_TWHR and nand_whr_delay().
>>>       You can set this new flag if you want nand_command(_lp)
>>>       to insert tWHR delay where needed.
>>>
>>> 2/2 : Fix Denali setup_data_interface.
>>>       Boris' suggestion in v1 was a good reminder that
>>>       made me realize tCCS was missing in the driver.  Fix it now.
>>>
>>>
>>> Changes in v2:
>>>   - Add nand_whr_delay() helper
>>>     Wait for tWHR only for drivers that explicitly set NAND_WAIT_TWHR flag
>>>   - newly added
>>>
>>> Masahiro Yamada (2):
>>>   mtd: nand: wait for tWHR after NAND_CMD_STATUS / NAND_CMD_READID
>>
>> Hm, I thought you were introducing this to then use it in the denali
>> driver. Sorry, but I don't want to apply something that nobody needs.
>> If someone ever complains about a missing delay I'll point him to your
>> patch, but until then I'll keep the core unchanged.
> 
> Perhaps, Marc Gonzalez is the person.
> 
> 
> tango_nand.c is the only driver that sets NAND_WAIT_TCCS.
> 
> Now, there is completely no delay when reading out the ID.
> 
> 
> One safe change might be apply this patch,
> then set NAND_WAIT_TWHR to tango_nand.c
> 
> 
> I am guessing NAND_WAIT_TCCS was added for it.
> Theoretically, I do not see logical difference between tCCS and tWHR.
> 
> I am CCing Marc Gonzalez, the author of tango_nand.c

Hello Masahiro,

I remember having issues reading the ONFI ID when I was writing
the driver, a year ago. Sometimes, the first few bytes appeared
to be missing. This looked like a timing issue.

Adding the dev_ready call-back solved the problem. Do you think
that was by accident? When I have more time, I will test the 4.14
branch, to see if there are any issues with the current driver.

Regards.

Re: [PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Masahiro Yamada]

Hi Marc,

2017-10-04 20:05 GMT+09:00 Marc Gonzalez <marc_gonzalez@sigmadesigns.com>:
> On 29/09/2017 16:33, Masahiro Yamada wrote:
>
>> (+CC Marc Gonzalez)
>>
>> 2017-09-29 21:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>>> On Fri, 29 Sep 2017 19:38:38 +0900
>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>
>>>> 1/2 : add NAND_WAIT_TWHR and nand_whr_delay().
>>>>       You can set this new flag if you want nand_command(_lp)
>>>>       to insert tWHR delay where needed.
>>>>
>>>> 2/2 : Fix Denali setup_data_interface.
>>>>       Boris' suggestion in v1 was a good reminder that
>>>>       made me realize tCCS was missing in the driver.  Fix it now.
>>>>
>>>>
>>>> Changes in v2:
>>>>   - Add nand_whr_delay() helper
>>>>     Wait for tWHR only for drivers that explicitly set NAND_WAIT_TWHR flag
>>>>   - newly added
>>>>
>>>> Masahiro Yamada (2):
>>>>   mtd: nand: wait for tWHR after NAND_CMD_STATUS / NAND_CMD_READID
>>>
>>> Hm, I thought you were introducing this to then use it in the denali
>>> driver. Sorry, but I don't want to apply something that nobody needs.
>>> If someone ever complains about a missing delay I'll point him to your
>>> patch, but until then I'll keep the core unchanged.
>>
>> Perhaps, Marc Gonzalez is the person.
>>
>>
>> tango_nand.c is the only driver that sets NAND_WAIT_TCCS.
>>
>> Now, there is completely no delay when reading out the ID.
>>
>>
>> One safe change might be apply this patch,
>> then set NAND_WAIT_TWHR to tango_nand.c
>>
>>
>> I am guessing NAND_WAIT_TCCS was added for it.
>> Theoretically, I do not see logical difference between tCCS and tWHR.
>>
>> I am CCing Marc Gonzalez, the author of tango_nand.c
>
> Hello Masahiro,
>
> I remember having issues reading the ONFI ID when I was writing
> the driver, a year ago. Sometimes, the first few bytes appeared
> to be missing. This looked like a timing issue.
>
> Adding the dev_ready call-back solved the problem. Do you think
> that was by accident?

It is odd to use dev_ready() hook to insert delay for the READ ID command.
READ ID command never toggles the device's Ready/Busy# pin.


> When I have more time, I will test the 4.14
> branch, to see if there are any issues with the current driver.


Yeah, I highly recommend you to test your driver on the latest kernel.
I suspect it is broken because READ ID command in the generic hook
has absolutely zero delay.

As I proposed already, the correct fix it to wait for tWHR.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Marc Gonzalez]

On 13/10/2017 10:34, Masahiro Yamada wrote:

> 2017-10-04 20:05, Marc Gonzalez wrote:
> 
>> On 29/09/2017 16:33, Masahiro Yamada wrote:
>>
>>> tango_nand.c is the only driver that sets NAND_WAIT_TCCS.
>>>
>>> Now, there is completely no delay when reading out the ID.
>>>
>>>
>>> One safe change might be apply this patch,
>>> then set NAND_WAIT_TWHR to tango_nand.c
>>>
>>>
>>> I am guessing NAND_WAIT_TCCS was added for it.
>>> Theoretically, I do not see logical difference between tCCS and tWHR.
>>>
>>> I am CCing Marc Gonzalez, the author of tango_nand.c
>>
>> Hello Masahiro,
>>
>> I remember having issues reading the ONFI ID when I was writing
>> the driver, a year ago. Sometimes, the first few bytes appeared
>> to be missing. This looked like a timing issue.
>>
>> Adding the dev_ready call-back solved the problem. Do you think
>> that was by accident?
> 
> It is odd to use dev_ready() hook to insert delay for the READ ID command.
> READ ID command never toggles the device's Ready/Busy# pin.
> 
> 
>> When I have more time, I will test the 4.14
>> branch, to see if there are any issues with the current driver.
> 
> 
> Yeah, I highly recommend you to test your driver on the latest kernel.
> I suspect it is broken because READ ID command in the generic hook
> has absolutely zero delay.
> 
> As I proposed already, the correct fix it to wait for tWHR.

Hello Masahiro,

I checked out v4.14-rc5, imported the DMA driver (which is, unfortunately,
not upstream) and DT nodes.

Chip identification seems to work out-of-the-box at least on my dev board
with that specific NAND chip model:

[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Linux version 4.14.0-rc5 (mgonzalez@misti.france.sdesigns.com) (gcc version 7.1.1 20170707 (Linaro GCC 7.1-2017.08)) #2 SMP PREEMPT Thu Oct 19 16:36:36 CEST 2017
[    0.000000] CPU: ARMv7 Processor [413fc090] revision 0 (ARMv7), cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[    0.000000] OF: fdt: Machine model: Sigma Designs SMP8758 Vantage-1172 Rev E1
...
[    0.964542] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xdc
[    0.970951] nand: Micron MT29F4G08ABADAWP
[    0.974986] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
[    0.982632] Scanning device for bad blocks
[    1.231971] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xdc
[    1.238370] nand: Micron MT29F4G08ABADAWP
[    1.242405] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
[    1.250041] Scanning device for bad blocks


I don't know enough about NAND chips to tell if it works by accident,
or if this is expected. I seem to recall that the first few operations
are carried out at the slowest possible speed, until the core figures
out the best timings for maximum performance. Maybe my controller can
cope with no wait at the slow speeds...

Regards.


Results of some mtd tests:

# modprobe mtd_speedtest dev=1
[  462.394474] mtd_speedtest: MTD device: 1
[  462.398447] mtd_speedtest: MTD device size 536870912, eraseblock size 131072, page size 2048, count of eraseblocks 4096, pages per eraseblock 64, OOB size 64
[  462.413301] mtd_test: scanning for bad eraseblocks
[  462.419640] mtd_test: scanned 4096 eraseblocks, 0 are bad
[  465.321466] mtd_speedtest: testing eraseblock write speed
[  529.053883] mtd_speedtest: eraseblock write speed is 8227 KiB/s
[  529.059843] mtd_speedtest: testing eraseblock read speed
[  564.486643] mtd_speedtest: eraseblock read speed is 14801 KiB/s
[  567.642582] mtd_speedtest: testing page write speed
[  631.715184] mtd_speedtest: page write speed is 8183 KiB/s
[  631.720619] mtd_speedtest: testing page read speed
[  667.403583] mtd_speedtest: page read speed is 14694 KiB/s
[  670.560833] mtd_speedtest: testing 2 page write speed
[  734.453881] mtd_speedtest: 2 page write speed is 8206 KiB/s
[  734.459490] mtd_speedtest: testing 2 page read speed
[  770.031472] mtd_speedtest: 2 page read speed is 14741 KiB/s
[  770.037092] mtd_speedtest: Testing erase speed
[  773.196752] mtd_speedtest: erase speed is 166176 KiB/s
[  773.201920] mtd_speedtest: Testing 2x multi-block erase speed
[  774.815910] mtd_speedtest: 2x multi-block erase speed is 326049 KiB/s
[  774.822388] mtd_speedtest: Testing 4x multi-block erase speed
[  776.436174] mtd_speedtest: 4x multi-block erase speed is 326049 KiB/s
[  776.442652] mtd_speedtest: Testing 8x multi-block erase speed
[  778.055158] mtd_speedtest: 8x multi-block erase speed is 326455 KiB/s
[  778.061636] mtd_speedtest: Testing 16x multi-block erase speed
[  779.673697] mtd_speedtest: 16x multi-block erase speed is 326455 KiB/s
[  779.680262] mtd_speedtest: Testing 32x multi-block erase speed
[  781.292462] mtd_speedtest: 32x multi-block erase speed is 326455 KiB/s
[  781.299027] mtd_speedtest: Testing 64x multi-block erase speed
[  782.910486] mtd_speedtest: 64x multi-block erase speed is 326659 KiB/s
[  782.917051] mtd_speedtest: finished


# modprobe mtd_stresstest dev=1
[ 1021.866306] mtd_stresstest: MTD device: 1
[ 1021.870356] mtd_stresstest: MTD device size 536870912, eraseblock size 131072, page size 2048, count of eraseblocks 4096, pages per eraseblock 64, OOB size 64
[ 1021.886066] mtd_test: scanning for bad eraseblocks
[ 1021.892415] mtd_test: scanned 4096 eraseblocks, 0 are bad
[ 1021.897847] mtd_stresstest: doing operations
[ 1021.902144] mtd_stresstest: 0 operations done
[ 1032.537748] mtd_stresstest: 1024 operations done
[ 1043.112145] mtd_stresstest: 2048 operations done
[ 1053.744567] mtd_stresstest: 3072 operations done
[ 1063.928274] mtd_stresstest: 4096 operations done
[ 1074.518502] mtd_stresstest: 5120 operations done
[ 1084.890384] mtd_stresstest: 6144 operations done
[ 1095.128781] mtd_stresstest: 7168 operations done
[ 1105.454702] mtd_stresstest: 8192 operations done
[ 1115.597722] mtd_stresstest: 9216 operations done
[ 1122.858549] mtd_stresstest: finished, 10000 operations done

Re: [PATCH v2 0/2] mtd: nand: wait for tWHR, and fix the setup_data_interface of Denali

[Masahiro Yamada]

Hi Marc,

2017-10-19 23:58 GMT+09:00 Marc Gonzalez <marc_gonzalez@sigmadesigns.com>:
> On 13/10/2017 10:34, Masahiro Yamada wrote:
>
>> 2017-10-04 20:05, Marc Gonzalez wrote:
>>
>>> On 29/09/2017 16:33, Masahiro Yamada wrote:
>>>
>>>> tango_nand.c is the only driver that sets NAND_WAIT_TCCS.
>>>>
>>>> Now, there is completely no delay when reading out the ID.
>>>>
>>>>
>>>> One safe change might be apply this patch,
>>>> then set NAND_WAIT_TWHR to tango_nand.c
>>>>
>>>>
>>>> I am guessing NAND_WAIT_TCCS was added for it.
>>>> Theoretically, I do not see logical difference between tCCS and tWHR.
>>>>
>>>> I am CCing Marc Gonzalez, the author of tango_nand.c
>>>
>>> Hello Masahiro,
>>>
>>> I remember having issues reading the ONFI ID when I was writing
>>> the driver, a year ago. Sometimes, the first few bytes appeared
>>> to be missing. This looked like a timing issue.
>>>
>>> Adding the dev_ready call-back solved the problem. Do you think
>>> that was by accident?
>>
>> It is odd to use dev_ready() hook to insert delay for the READ ID command.
>> READ ID command never toggles the device's Ready/Busy# pin.
>>
>>
>>> When I have more time, I will test the 4.14
>>> branch, to see if there are any issues with the current driver.
>>
>>
>> Yeah, I highly recommend you to test your driver on the latest kernel.
>> I suspect it is broken because READ ID command in the generic hook
>> has absolutely zero delay.
>>
>> As I proposed already, the correct fix it to wait for tWHR.
>
> Hello Masahiro,
>
> I checked out v4.14-rc5, imported the DMA driver (which is, unfortunately,
> not upstream) and DT nodes.


Thanks for your test report.

I was worried since you said
you had issues reading the ONFI ID.

Anyway, it is OK if your driver is working.

Thanks.




-- 
Best Regards
Masahiro Yamada