[PATCH v2 0/3] mtd: nand: introduce a READMODE command

[PATCH v2 0/3] mtd: nand: introduce a READMODE command

[Gerhard Sittig]

this series does some minor cleanup in nand_command_lp() and then
introduces a NAND_CMD_READMODE opcode which is similar to the existing
NAND_CMD_READ0 but does not imply NAND_CMD_READSTART

such a READMODE command is useful in preparation of the pending
introduction of on-die-ECC support

changes in v2:
- apply command bit masking in nand_command() as well, such that all
  cmdfunc() variants can accept all defined NAND commands
- reword the command bit masking comments and align their versions in
  the large page and non-large page cases
- rephrase the NAND_CMD_READMODE declaration to better reflect that it
  re-uses the NAND_CMD_READ0 opcode
- update commit messages, mention that for nand_command() the READMODE
  and READ0 commands result in identical behaviour, because there is no
  READSTART involved

Gerhard Sittig (3):
  mtd: nand: comment update, the DEPLETE1 command has gone
  mtd: nand: re-introduce command bits masking
  mtd: nand: introduce a READMODE command

 drivers/mtd/nand/nand_base.c |   23 ++++++++++++++++++-----
 include/linux/mtd/nand.h     |   11 +++++++++++
 2 files changed, 29 insertions(+), 5 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/3] mtd: nand: comment update, the DEPLETE1 command has gone

[Gerhard Sittig]

update a comment in nand_command_lp() about specific requirements of
individual commands, the DEPLETE1 command was removed in the past and
the comment no longer applied

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
changes in v2: none

 drivers/mtd/nand/nand_base.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 9715a7ba164a..567620e81ce2 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -700,7 +700,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 
 	/*
 	 * Program and erase have their own busy handlers status, sequential
-	 * in, and deplete1 need no delay.
+	 * in and status need no delay.
 	 */
 	switch (command) {
 
-- 
1.7.10.4

Re: [PATCH v2 1/3] mtd: nand: comment update, the DEPLETE1 command has gone

[Brian Norris]

On Fri, Apr 04, 2014 at 01:45:46PM +0200, Gerhard Sittig wrote:
> update a comment in nand_command_lp() about specific requirements of
> individual commands, the DEPLETE1 command was removed in the past and
> the comment no longer applied
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
> changes in v2: none

Pushed this patch to l2-mtd.git.

Brian

[PATCH v2 2/3] mtd: nand: re-introduce command bits masking

[Gerhard Sittig]

re-introduce the mask operation which was removed in fb066adadd22
"mtd: nand_base: Removed unnecessary command masking" after the DEPLETE1
command has gone and the masking no longer was strictly required

keeping the operation here is cheap and does not influence behaviour as
long as all passed in command opcodes are within a byte's range

pending introduction of on-die-ECC support needs a "READMODE" command,
which shares a lot of the READ0 code path, yet would require duplication
or open coding if "READMODE" (aka "READ0 exclusively") cannot be told from
"READPAGE" (aka "the READ0 and READSTART sequence")

(the "READPAGE" and "READMODE" terms are coined in Micron datasheets)

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
changes in v2:
- mask command bits in nand_command() as well, such that large page and
  non-large page configurations can accept all defined commands
- reword comments and align their nand_command() and nand_command_lp()
  versions

 drivers/mtd/nand/nand_base.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 567620e81ce2..7108191b1598 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -564,7 +564,12 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 	register struct nand_chip *chip = mtd->priv;
 	int ctrl = NAND_CTRL_CLE | NAND_CTRL_CHANGE;
 
-	/* Write out the command to the device */
+	/*
+	 * Command latch cycle, write out the command's lower bits
+	 * to the device, followed by optional address specs and
+	 * subsequent commands, while the command's higher bits
+	 * might modify the below logic of post-operation and delays
+	 */
 	if (command == NAND_CMD_SEQIN) {
 		int readcmd;
 
@@ -582,7 +587,7 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 		chip->cmd_ctrl(mtd, readcmd, ctrl);
 		ctrl &= ~NAND_CTRL_CHANGE;
 	}
-	chip->cmd_ctrl(mtd, command, ctrl);
+	chip->cmd_ctrl(mtd, command & 0xff, ctrl);
 
 	/* Address cycle, when necessary */
 	ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE;
@@ -671,8 +676,14 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 		command = NAND_CMD_READ0;
 	}
 
-	/* Command latch cycle */
-	chip->cmd_ctrl(mtd, command, NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
+	/*
+	 * Command latch cycle, write out the command's lower bits
+	 * to the device, followed by optional address specs and
+	 * subsequent commands, while the command's higher bits
+	 * might modify the below logic of post-operation and delays
+	 */
+	chip->cmd_ctrl(mtd, command & 0xff,
+		       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
 
 	if (column != -1 || page_addr != -1) {
 		int ctrl = NAND_CTRL_CHANGE | NAND_NCE | NAND_ALE;
-- 
1.7.10.4

[PATCH v2 3/3] mtd: nand: introduce a READMODE command

[Gerhard Sittig]

the nand_command_lp() implementation derives a "READPAGE" sequence from
a passed in READ0 opcode, i.e. emits a sequence of READ0 _and_ READSTART
commands in this case

introduce a "READMODE" command which sends the READ0 opcode to the chip
exclusively and doesn't send the READSTART opcode

such a "READMODE" command is useful in the context of on-die-ECC support
where a sequence of READ0, READSTART, STATUS, READ0 is required; having
support for READMODE in the common nand_command_lp() routine avoids the
need for duplication and open coded cmd_ctrl() calls

for the non-"large page" setup (i.e. the nand_command() routine) both
commands "READMODE" and "READ0" are identical, "READSTART" exclusively
applies to large page configurations

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
changes in v2:
- update the commmit message to discuss that for the nand_command()
  routine READMODE results in identical behaviour as READ0
- rephrase the NAND_CMD_READMODE command to better reflect that it
  re-uses the NAND_CMD_READ0 opcode plus has high bits set

 drivers/mtd/nand/nand_base.c |    4 +++-
 include/linux/mtd/nand.h     |   11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7108191b1598..50b8a2a93b4f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -711,7 +711,8 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 
 	/*
 	 * Program and erase have their own busy handlers status, sequential
-	 * in and status need no delay.
+	 * in and status need no delay, read mode just reverts back to
+	 * data output after a status command and needs no read start.
 	 */
 	switch (command) {
 
@@ -722,6 +723,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 	case NAND_CMD_SEQIN:
 	case NAND_CMD_RNDIN:
 	case NAND_CMD_STATUS:
+	case NAND_CMD_READMODE:
 		return;
 
 	case NAND_CMD_RESET:
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 32f8612469d8..f294c9a47143 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -106,6 +106,17 @@ extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 
 #define NAND_CMD_NONE		-1
 
+/*
+ * Pseudo commands, which map to the above "real" commands for the NAND chip
+ * yet involve slightly different behaviour in related software layers
+ *
+ * READMODE switches back from status output to data output after a
+ * previously emitted sequence of READ0, READSTART, and STATUS commands;
+ * actually it's a mere READ0 without the address specs and without the
+ * READSTART command which the READ0 convenience logic would imply
+ */
+#define NAND_CMD_READMODE	(0x100 | NAND_CMD_READ0)
+
 /* Status bits */
 #define NAND_STATUS_FAIL	0x01
 #define NAND_STATUS_FAIL_N1	0x02
-- 
1.7.10.4

RE: [PATCH v2 3/3] mtd: nand: introduce a READMODE command

[Gupta, Pekon]

Hi Gerhard,

>From: Gerhard Sittig [mailto:gsi@denx.de]
>
>the nand_command_lp() implementation derives a "READPAGE" sequence from
>a passed in READ0 opcode, i.e. emits a sequence of READ0 _and_ READSTART
>commands in this case
>
>introduce a "READMODE" command which sends the READ0 opcode to the chip
>exclusively and doesn't send the READSTART opcode
>
>such a "READMODE" command is useful in the context of on-die-ECC support
>where a sequence of READ0, READSTART, STATUS, READ0 is required; having
>support for READMODE in the common nand_command_lp() routine avoids the
>need for duplication and open coded cmd_ctrl() calls
>
>for the non-"large page" setup (i.e. the nand_command() routine) both
>commands "READMODE" and "READ0" are identical, "READSTART" exclusively
>applies to large page configurations
>
>Signed-off-by: Gerhard Sittig <gsi@denx.de>
>---
>changes in v2:
>- update the commmit message to discuss that for the nand_command()
>  routine READMODE results in identical behaviour as READ0
>- rephrase the NAND_CMD_READMODE command to better reflect that it
>  re-uses the NAND_CMD_READ0 opcode plus has high bits set
>
> drivers/mtd/nand/nand_base.c |    4 +++-
> include/linux/mtd/nand.h     |   11 +++++++++++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>index 7108191b1598..50b8a2a93b4f 100644
>--- a/drivers/mtd/nand/nand_base.c
>+++ b/drivers/mtd/nand/nand_base.c
>@@ -711,7 +711,8 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
>
> 	/*
> 	 * Program and erase have their own busy handlers status, sequential
>-	 * in and status need no delay.
>+	 * in and status need no delay, read mode just reverts back to
>+	 * data output after a status command and needs no read start.
> 	 */
> 	switch (command) {
>
>@@ -722,6 +723,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
> 	case NAND_CMD_SEQIN:
> 	case NAND_CMD_RNDIN:
> 	case NAND_CMD_STATUS:
>+	case NAND_CMD_READMODE:
> 		return;
>
> 	case NAND_CMD_RESET:

You probably missed doing same change in nand_command() also.

with regards, pekon

Re: [PATCH v2 3/3] mtd: nand: introduce a READMODE command

[Gerhard Sittig]

On Tue, 2014-04-15 at 04:10 +0000, Gupta, Pekon wrote:
> 
> Hi Gerhard,
> 
> >From: Gerhard Sittig [mailto:gsi@denx.de]
> >
> >the nand_command_lp() implementation derives a "READPAGE" sequence from
> >a passed in READ0 opcode, i.e. emits a sequence of READ0 _and_ READSTART
> >commands in this case
> >
> >introduce a "READMODE" command which sends the READ0 opcode to the chip
> >exclusively and doesn't send the READSTART opcode
> >
> >such a "READMODE" command is useful in the context of on-die-ECC support
> >where a sequence of READ0, READSTART, STATUS, READ0 is required; having
> >support for READMODE in the common nand_command_lp() routine avoids the
> >need for duplication and open coded cmd_ctrl() calls
> >
> >for the non-"large page" setup (i.e. the nand_command() routine) both
> >commands "READMODE" and "READ0" are identical, "READSTART" exclusively
> >applies to large page configurations
> >
> >Signed-off-by: Gerhard Sittig <gsi@denx.de>
> >---
> >changes in v2:
> >- update the commmit message to discuss that for the nand_command()
> >  routine READMODE results in identical behaviour as READ0
> >- rephrase the NAND_CMD_READMODE command to better reflect that it
> >  re-uses the NAND_CMD_READ0 opcode plus has high bits set
> >
> > drivers/mtd/nand/nand_base.c |    4 +++-
> > include/linux/mtd/nand.h     |   11 +++++++++++
> > 2 files changed, 14 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >index 7108191b1598..50b8a2a93b4f 100644
> >--- a/drivers/mtd/nand/nand_base.c
> >+++ b/drivers/mtd/nand/nand_base.c
> >@@ -711,7 +711,8 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
> >
> > 	/*
> > 	 * Program and erase have their own busy handlers status, sequential
> >-	 * in and status need no delay.
> >+	 * in and status need no delay, read mode just reverts back to
> >+	 * data output after a status command and needs no read start.
> > 	 */
> > 	switch (command) {
> >
> >@@ -722,6 +723,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
> > 	case NAND_CMD_SEQIN:
> > 	case NAND_CMD_RNDIN:
> > 	case NAND_CMD_STATUS:
> >+	case NAND_CMD_READMODE:
> > 		return;
> >
> > 	case NAND_CMD_RESET:
> 
> You probably missed doing same change in nand_command() also.

Thank you for looking at the patches, again!

AFAICS nothing is missing, the difference is there by purpose.

The READMODE command does not require special handling in the
nand_command() case, as the sequence of READ0, READSTART, STATUS,
READ0 does not apply there (it's only relevant to large page
setups).

In the nand_command() case the sequence would be READ0, STATUS,
READ0 -- and so READ0 and READMODE are handled identically,
READSTART does not exist in that scenario.  And the STATUS and
READ0 only would apply in the on-die-ECC scenario, which might as
well not occur at all without large pages.

I tried to reason about this situation in the commit message, but
might not have come up with the best phrases there.  (That's the
reason for the excessive quotation above.)

There is a difference between nand_command_lp() and
nand_command() in that for the latter the re-emitted READMODE
will result in another busy check (or a delay in the absence of
an R/B pin condition).  So we fail on the safe side, which should
be OK.

Would an improved commit message help?  Or shall I put an
explicit comment into nand_command() about why it's different
from nand_command_lp()?  Although it already is, the situation is
not new ...


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

RE: [PATCH v2 3/3] mtd: nand: introduce a READMODE command

[Gupta, Pekon]

Hi Gerhard,

>From: Gerhard Sittig [mailto:gsi@denx.de]
>>On Tue, 2014-04-15 at 04:10 +0000, Gupta, Pekon wrote:
>>
>> Hi Gerhard,
>>
>> >From: Gerhard Sittig [mailto:gsi@denx.de]
>> >
>> >the nand_command_lp() implementation derives a "READPAGE" sequence from
>> >a passed in READ0 opcode, i.e. emits a sequence of READ0 _and_ READSTART
>> >commands in this case
>> >
>> >introduce a "READMODE" command which sends the READ0 opcode to the chip
>> >exclusively and doesn't send the READSTART opcode
>> >
>> >such a "READMODE" command is useful in the context of on-die-ECC support
>> >where a sequence of READ0, READSTART, STATUS, READ0 is required; having
>> >support for READMODE in the common nand_command_lp() routine avoids the
>> >need for duplication and open coded cmd_ctrl() calls
>> >
>> >for the non-"large page" setup (i.e. the nand_command() routine) both
>> >commands "READMODE" and "READ0" are identical, "READSTART" exclusively
>> >applies to large page configurations
>> >
>> >Signed-off-by: Gerhard Sittig <gsi@denx.de>
>> >---
>> >changes in v2:
>> >- update the commmit message to discuss that for the nand_command()
>> >  routine READMODE results in identical behaviour as READ0
>> >- rephrase the NAND_CMD_READMODE command to better reflect that it
>> >  re-uses the NAND_CMD_READ0 opcode plus has high bits set
>> >
>> > drivers/mtd/nand/nand_base.c |    4 +++-
>> > include/linux/mtd/nand.h     |   11 +++++++++++
>> > 2 files changed, 14 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> >index 7108191b1598..50b8a2a93b4f 100644
>> >--- a/drivers/mtd/nand/nand_base.c
>> >+++ b/drivers/mtd/nand/nand_base.c
>> >@@ -711,7 +711,8 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
>> >
>> > 	/*
>> > 	 * Program and erase have their own busy handlers status, sequential
>> >-	 * in and status need no delay.
>> >+	 * in and status need no delay, read mode just reverts back to
>> >+	 * data output after a status command and needs no read start.
>> > 	 */
>> > 	switch (command) {
>> >
>> >@@ -722,6 +723,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
>> > 	case NAND_CMD_SEQIN:
>> > 	case NAND_CMD_RNDIN:
>> > 	case NAND_CMD_STATUS:
>> >+	case NAND_CMD_READMODE:
>> > 		return;
>> >
>> > 	case NAND_CMD_RESET:
>>
>> You probably missed doing same change in nand_command() also.
>
>Thank you for looking at the patches, again!
>
>AFAICS nothing is missing, the difference is there by purpose.
>
>The READMODE command does not require special handling in the
>nand_command() case, as the sequence of READ0, READSTART, STATUS,
>READ0 does not apply there (it's only relevant to large page
>setups).
>
>In the nand_command() case the sequence would be READ0, STATUS,
>READ0 -- and so READ0 and READMODE are handled identically,
>READSTART does not exist in that scenario.  And the STATUS and
>READ0 only would apply in the on-die-ECC scenario, which might as
>well not occur at all without large pages.
>
>I tried to reason about this situation in the commit message, but
>might not have come up with the best phrases there.  (That's the
>reason for the excessive quotation above.)
>
>There is a difference between nand_command_lp() and
>nand_command() in that for the latter the re-emitted READMODE
>will result in another busy check (or a delay in the absence of
>an R/B pin condition).  So we fail on the safe side, which should
>be OK.
>
Ok thanks for the explanation, I missed reading your new commit message.
Though I don't have any small-page NAND device datasheet, but from the
code I can see the difference in implementation of nand_command() and
nand_command_lp() w.r.t. NAND_CMD_READ0 (0x00).

Probably for small-page devices, NAND_CMD_READ0 (0x00) need
not be followed by NAND_CMD_READSTART(0x30). So, for small-page
devices NAND_CMD_READ0(0x00) itself will act as NAND_CMD_READMODE.


>Would an improved commit message help?  Or shall I put an
>explicit comment into nand_command() about why it's different
>from nand_command_lp()?  Although it already is, the situation is
>not new ...
>
I'll let Brian review it once. But from my side ...

Acked-by: Pekon Gupta <pekon@ti.com>

>
>virtually yours
>Gerhard Sittig
>--

Thanks.
with regards, pekon

Re: [PATCH v2 0/3] mtd: nand: introduce a READMODE command

[Gerhard Sittig]

On Fri, 2014-04-04 at 13:45 +0200, Gerhard Sittig wrote:
> 
> this series does some minor cleanup in nand_command_lp() and then
> introduces a NAND_CMD_READMODE opcode which is similar to the existing
> NAND_CMD_READ0 but does not imply NAND_CMD_READSTART
> 
> such a READMODE command is useful in preparation of the pending
> introduction of on-die-ECC support

Are there any concerns left that I should address?


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH v2 0/3] mtd: nand: introduce a READMODE command

[Gerhard Sittig]

On Mon, 2014-04-14 at 20:55 +0200, Gerhard Sittig wrote:
> 
> On Fri, 2014-04-04 at 13:45 +0200, Gerhard Sittig wrote:
> > 
> > this series does some minor cleanup in nand_command_lp() and then
> > introduces a NAND_CMD_READMODE opcode which is similar to the existing
> > NAND_CMD_READ0 but does not imply NAND_CMD_READSTART
> > 
> > such a READMODE command is useful in preparation of the pending
> > introduction of on-die-ECC support
> 
> Are there any concerns left that I should address?

There have been two good weeks without further comments, and an
ACK from Gupta Pekon.  Can the series get applied?  Should I
re-submit with the ACK?

I assume that trackers already did catch the ACK, so I only would
duplicate their action and re-send what has not changed.  And I
understand that the delay is because of the merge window.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH v2 0/3] mtd: nand: introduce a READMODE command

[Gerhard Sittig]

On Mon, 2014-05-05 at 15:29 +0200, Gerhard Sittig wrote:
> 
> On Mon, 2014-04-14 at 20:55 +0200, Gerhard Sittig wrote:
> > 
> > On Fri, 2014-04-04 at 13:45 +0200, Gerhard Sittig wrote:
> > > 
> > > this series does some minor cleanup in nand_command_lp() and then
> > > introduces a NAND_CMD_READMODE opcode which is similar to the existing
> > > NAND_CMD_READ0 but does not imply NAND_CMD_READSTART
> > > 
> > > such a READMODE command is useful in preparation of the pending
> > > introduction of on-die-ECC support
> > 
> > Are there any concerns left that I should address?
> 
> There have been two good weeks without further comments, and an
> ACK from Gupta Pekon.  Can the series get applied?  Should I
> re-submit with the ACK?

Are there any remaining concerns that I shall address?  Can the
series get picked up?  Shall I re-send?  Thank you for checking!


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de