[v1,3/3] mtd: nand: introduce a READMODE command

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

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 drivers/mtd/nand/nand_base.c |    4 +++-
 include/linux/mtd/nand.h     |   11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Gerhard,

Thanks for putting these patches together!

Would it be cleaner to define READMODE like this:

#define NAND_CMD_READMODE      (0x100 | NAND_CMD_READ0)

?

I'm updating my patch to take advantage of READMODE.

  --david

On Mon, 2014-03-31 at 11:50 -0600, David Mosberger-Tang wrote:
> 
> Would it be cleaner to define READMODE like this:
> 
> #define NAND_CMD_READMODE      (0x100 | NAND_CMD_READ0)

See 'git show 0be718e5525a' (the comments suggest that I took the
DEPLETE1 command as an inspiration), the code was not constructed
there either.  But I'm open to suggestions, let's see what others
think or whether there already is an established preference for
such aggregates.


> I'm updating my patch to take advantage of READMODE.

Can you identify other stuff that could get built in steps, to
chop the big change into pieces that are a little easier to
review and verify, to help getting your on-die-ECC introduction
accepted?

Will the better mapping to the current flow of command emission,
data fetching, and optional ECC handling become "a byproduct" of
constructing the total support from incremental building blocks?
Like starting with the "supported?" check, adding an "enabled?"
check, adding read/write support with mere boolean error
conditions, then add the re-read and bitflip count dance?

Splitting complex changes into logical groups helps in later
maintenance, too (bisection to identify and isolate problems,
extending or fixing clearly identified spots of restricted
functionality).


virtually yours
Gerhard Sittig

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
>
>Signed-off-by: Gerhard Sittig <gsi@denx.de>
>---
> 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 9ae99c1ba772..9a74bd06004b 100644
>--- a/drivers/mtd/nand/nand_base.c
>+++ b/drivers/mtd/nand/nand_base.c
>@@ -706,7 +706,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) {
>
>@@ -717,6 +718,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;
>

Thanks. This should help 'David Mosberger <davidm@egauge.net>' patches.
I think, NAND_CMD_READMODE should also be added to nand_command(), right ?  
With that please feel free to apply ..
Acked-by: Pekon Gupta <pekon@ti.com>

with regards, pekon

On Tue, 2014-04-01 at 06:56 +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
> >
> >[ ... ]
> 
> Thanks. This should help 'David Mosberger <davidm@egauge.net>' patches.
> I think, NAND_CMD_READMODE should also be added to nand_command(), right ?  

Oh, that has slipped my attention.  I'll wait a few more days for
feedback, and update the series to support READMODE in both the
nand_command() as well as nand_command_lp() variants.  Callers
should not mind which one is the registered cmdfunc() callback.

> With that please feel free to apply ..
> Acked-by: Pekon Gupta <pekon@ti.com>

thank you, both for looking and for the ACK


virtually yours
Gerhard Sittig