Patchwork [06/12] mtd: nand: remove a bunch of unused commands

login
register
mail settings
Submitter Artem Bityutskiy
Date March 4, 2013, 4:42 p.m.
Message ID <1362415349-7107-7-git-send-email-dedekind1@gmail.com>
Download mbox | patch
Permalink /patch/224762/
State New
Headers show

Comments

Artem Bityutskiy - March 4, 2013, 4:42 p.m.
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/nand/cafe_nand.c   |    6 ------
 drivers/mtd/nand/nand_base.c   |   10 ----------
 drivers/mtd/nand/nandsim.c     |    8 --------
 drivers/mtd/nand/nuc900_nand.c |    9 ---------
 include/linux/mtd/nand.h       |   20 --------------------
 5 files changed, 53 deletions(-)
Brian Norris - March 4, 2013, 7:04 p.m.
+ Alexander

On Mon, Mar 4, 2013 at 8:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  drivers/mtd/nand/cafe_nand.c   |    6 ------
>  drivers/mtd/nand/nand_base.c   |   10 ----------
>  drivers/mtd/nand/nandsim.c     |    8 --------
>  drivers/mtd/nand/nuc900_nand.c |    9 ---------
>  include/linux/mtd/nand.h       |   20 --------------------
>  5 files changed, 53 deletions(-)
>

... trimmed ...

> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 2aba049..69097e8 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -86,7 +86,6 @@ extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  #define NAND_CMD_READOOB       0x50
>  #define NAND_CMD_ERASE1                0x60
>  #define NAND_CMD_STATUS                0x70
> -#define NAND_CMD_STATUS_MULTI  0x71
>  #define NAND_CMD_SEQIN         0x80
>  #define NAND_CMD_RNDIN         0x85
>  #define NAND_CMD_READID                0x90
> @@ -105,25 +104,6 @@ extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  #define NAND_CMD_RNDOUTSTART   0xE0
>  #define NAND_CMD_CACHEDPROG    0x15
>
> -/* Extended commands for AG-AND device */
> -/*
> - * Note: the command for NAND_CMD_DEPLETE1 is really 0x00 but
> - *       there is no way to distinguish that from NAND_CMD_READ0
> - *       until the remaining sequence of commands has been completed
> - *       so add a high order bit and mask it off in the command.
> - */
> -#define NAND_CMD_DEPLETE1      0x100

Perhaps this is the reason for the "unnecessary command masking" noted
by Alexander? There is one instance of a command function which masks
command & 0xff. Maybe Alexander's patch can be updated to mention this
likely cause for the original masking and can be applied on top of
Artem's cleanup series? Anyway, the mask was likely not used anyway,
since (as Artem mentions in this patch series) AG-AND had very little
general use (or none?).

> -#define NAND_CMD_DEPLETE2      0x38
> -#define NAND_CMD_STATUS_MULTI  0x71
> -#define NAND_CMD_STATUS_ERROR  0x72
> -/* multi-bank error status (banks 0-3) */
> -#define NAND_CMD_STATUS_ERROR0 0x73
> -#define NAND_CMD_STATUS_ERROR1 0x74
> -#define NAND_CMD_STATUS_ERROR2 0x75
> -#define NAND_CMD_STATUS_ERROR3 0x76
> -#define NAND_CMD_STATUS_RESET  0x7f
> -#define NAND_CMD_STATUS_CLEAR  0xff
> -
>  #define NAND_CMD_NONE          -1
>
>  /* Status bits */

Brian
Alexander Shiyan - March 4, 2013, 7:29 p.m.
> On Mon, Mar 4, 2013 at 8:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> >
> > Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> > ---
> >  drivers/mtd/nand/cafe_nand.c   |    6 ------
> >  drivers/mtd/nand/nand_base.c   |   10 ----------
> >  drivers/mtd/nand/nandsim.c     |    8 --------
> >  drivers/mtd/nand/nuc900_nand.c |    9 ---------
> >  include/linux/mtd/nand.h       |   20 --------------------
> >  5 files changed, 53 deletions(-)
> >
> 
> ... trimmed ...
...
> > - * Note: the command for NAND_CMD_DEPLETE1 is really 0x00 but
> > - *       there is no way to distinguish that from NAND_CMD_READ0
> > - *       until the remaining sequence of commands has been completed
> > - *       so add a high order bit and mask it off in the command.
> > - */
> > -#define NAND_CMD_DEPLETE1      0x100
> 
> Perhaps this is the reason for the "unnecessary command masking" noted
> by Alexander? There is one instance of a command function which masks
> command & 0xff. Maybe Alexander's patch can be updated to mention this
> likely cause for the original masking and can be applied on top of
> Artem's cleanup series? Anyway, the mask was likely not used anyway,
> since (as Artem mentions in this patch series) AG-AND had very little
> general use (or none?).

My patch is cleanup only. I think that the mask was originally included to
limit the byte boundaries. However, this is not necessary because callback
functions "cmd_ctrl" use writeb/iowrite8 for send command, ie do not allow
the use of the wrong size. Address also passed to "cmd_ctrl" via "cmd"
parameter, which is not masked but only shifted, i.e. we have a values
above 0xff, so that is not a problem.

---
Brian Norris - March 4, 2013, 7:54 p.m.
On Mon, Mar 4, 2013 at 11:29 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
>> On Mon, Mar 4, 2013 at 8:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> >
>> > Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> > ---
>> >  drivers/mtd/nand/cafe_nand.c   |    6 ------
>> >  drivers/mtd/nand/nand_base.c   |   10 ----------
>> >  drivers/mtd/nand/nandsim.c     |    8 --------
>> >  drivers/mtd/nand/nuc900_nand.c |    9 ---------
>> >  include/linux/mtd/nand.h       |   20 --------------------
>> >  5 files changed, 53 deletions(-)
>> >
>>
>> ... trimmed ...
> ...
>> > - * Note: the command for NAND_CMD_DEPLETE1 is really 0x00 but
>> > - *       there is no way to distinguish that from NAND_CMD_READ0
>> > - *       until the remaining sequence of commands has been completed
>> > - *       so add a high order bit and mask it off in the command.
>> > - */
>> > -#define NAND_CMD_DEPLETE1      0x100
>>
>> Perhaps this is the reason for the "unnecessary command masking" noted
>> by Alexander? There is one instance of a command function which masks
>> command & 0xff. Maybe Alexander's patch can be updated to mention this
>> likely cause for the original masking and can be applied on top of
>> Artem's cleanup series? Anyway, the mask was likely not used anyway,
>> since (as Artem mentions in this patch series) AG-AND had very little
>> general use (or none?).
>
> My patch is cleanup only. I think that the mask was originally included to
> limit the byte boundaries. However, this is not necessary because callback
> functions "cmd_ctrl" use writeb/iowrite8 for send command, ie do not allow
> the use of the wrong size. Address also passed to "cmd_ctrl" via "cmd"
> parameter, which is not masked but only shifted, i.e. we have a values
> above 0xff, so that is not a problem.

To be clear, I'm referring to your patch:

  [PATCH 1/3] mtd: nand_base: Removed unnecessary command masking

The function that you are editing (nand_command_lp) can be used for
AG-AND devices. The driver (rtc_from4.c - slated for removal in this
series) can send NAND_CMD_DEPLETE1, which according to the comments in
nand.h that I highlight above, should be "mask[ed] ... off in the
command". So without Artem's change, your patch is actually breaking
rtc_from4.c. Unforunately, no one bothered to actually document this
within nand_command_lp, but such is life.

Brian
Alexander Shiyan - March 4, 2013, 8:03 p.m.
> On Mon, Mar 4, 2013 at 11:29 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> >> On Mon, Mar 4, 2013 at 8:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> >> >
> >> > Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> >> > ---
> >> >  drivers/mtd/nand/cafe_nand.c   |    6 ------
> >> >  drivers/mtd/nand/nand_base.c   |   10 ----------
> >> >  drivers/mtd/nand/nandsim.c     |    8 --------
> >> >  drivers/mtd/nand/nuc900_nand.c |    9 ---------
> >> >  include/linux/mtd/nand.h       |   20 --------------------
> >> >  5 files changed, 53 deletions(-)
> >> >
> >>
> >> ... trimmed ...
> > ...
> >> > - * Note: the command for NAND_CMD_DEPLETE1 is really 0x00 but
> >> > - *       there is no way to distinguish that from NAND_CMD_READ0
> >> > - *       until the remaining sequence of commands has been completed
> >> > - *       so add a high order bit and mask it off in the command.
> >> > - */
> >> > -#define NAND_CMD_DEPLETE1      0x100
> >>
> >> Perhaps this is the reason for the "unnecessary command masking" noted
> >> by Alexander? There is one instance of a command function which masks
> >> command & 0xff. Maybe Alexander's patch can be updated to mention this
> >> likely cause for the original masking and can be applied on top of
> >> Artem's cleanup series? Anyway, the mask was likely not used anyway,
> >> since (as Artem mentions in this patch series) AG-AND had very little
> >> general use (or none?).
> >
> > My patch is cleanup only. I think that the mask was originally included to
> > limit the byte boundaries. However, this is not necessary because callback
> > functions "cmd_ctrl" use writeb/iowrite8 for send command, ie do not allow
> > the use of the wrong size. Address also passed to "cmd_ctrl" via "cmd"
> > parameter, which is not masked but only shifted, i.e. we have a values
> > above 0xff, so that is not a problem.
> 
> To be clear, I'm referring to your patch:
> 
>   [PATCH 1/3] mtd: nand_base: Removed unnecessary command masking
> 
> The function that you are editing (nand_command_lp) can be used for

OK, for Samsung chips too.

> AG-AND devices. The driver (rtc_from4.c - slated for removal in this
> series) can send NAND_CMD_DEPLETE1, which according to the comments in
> nand.h that I highlight above, should be "mask[ed] ... off in the
> command". So without Artem's change, your patch is actually breaking

Where it breaks? "rtc_from4_hwcontrol" use writeb() for "cmd" parameter,
so its not breaks. Is I think incorrect?

> rtc_from4.c. Unforunately, no one bothered to actually document this
> within nand_command_lp, but such is life.

---
Brian Norris - March 4, 2013, 8:27 p.m.
On Mon, Mar 4, 2013 at 12:03 PM, Alexander Shiyan <shc_work@mail.ru> wrote:
>> On Mon, Mar 4, 2013 at 11:29 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
>> >> On Mon, Mar 4, 2013 at 8:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> >> > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> >> >
>> >> > Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> >> > ---
>> >> >  drivers/mtd/nand/cafe_nand.c   |    6 ------
>> >> >  drivers/mtd/nand/nand_base.c   |   10 ----------
>> >> >  drivers/mtd/nand/nandsim.c     |    8 --------
>> >> >  drivers/mtd/nand/nuc900_nand.c |    9 ---------
>> >> >  include/linux/mtd/nand.h       |   20 --------------------
>> >> >  5 files changed, 53 deletions(-)
>> >> >
>> >>
>> >> ... trimmed ...
>> > ...
>> >> > - * Note: the command for NAND_CMD_DEPLETE1 is really 0x00 but
>> >> > - *       there is no way to distinguish that from NAND_CMD_READ0
>> >> > - *       until the remaining sequence of commands has been completed
>> >> > - *       so add a high order bit and mask it off in the command.
>> >> > - */
>> >> > -#define NAND_CMD_DEPLETE1      0x100
>> >>
>> >> Perhaps this is the reason for the "unnecessary command masking" noted
>> >> by Alexander? There is one instance of a command function which masks
>> >> command & 0xff. Maybe Alexander's patch can be updated to mention this
>> >> likely cause for the original masking and can be applied on top of
>> >> Artem's cleanup series? Anyway, the mask was likely not used anyway,
>> >> since (as Artem mentions in this patch series) AG-AND had very little
>> >> general use (or none?).
>> >
>> > My patch is cleanup only. I think that the mask was originally included to
>> > limit the byte boundaries. However, this is not necessary because callback
>> > functions "cmd_ctrl" use writeb/iowrite8 for send command, ie do not allow
>> > the use of the wrong size. Address also passed to "cmd_ctrl" via "cmd"
>> > parameter, which is not masked but only shifted, i.e. we have a values
>> > above 0xff, so that is not a problem.
>>
>> To be clear, I'm referring to your patch:
>>
>>   [PATCH 1/3] mtd: nand_base: Removed unnecessary command masking
>>
>> The function that you are editing (nand_command_lp) can be used for
>
> OK, for Samsung chips too.

Sure, and many others. But the AG-AND are the only case where you
would use a command that is not 8 bits anyway.

>> AG-AND devices. The driver (rtc_from4.c - slated for removal in this
>> series) can send NAND_CMD_DEPLETE1, which according to the comments in
>> nand.h that I highlight above, should be "mask[ed] ... off in the
>> command". So without Artem's change, your patch is actually breaking
>
> Where it breaks? "rtc_from4_hwcontrol" use writeb() for "cmd" parameter,
> so its not breaks. Is I think incorrect?

I see. I guess it is correct, but it still hard to follow the logic
that leads me to believe that this is a 100% correct change. I would
appreciate it if:

(1) We remove all commands that are larger than 8-bit (as with Artem's
current patch set)
(2) Alexander explains his change in more detail.

Anyway, I will move discussion of his changes back to his patch set, not here.

>> rtc_from4.c. Unforunately, no one bothered to actually document this
>> within nand_command_lp, but such is life.
>
> ---

Brian

Patch

diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
index 010d612..a01ccb9 100644
--- a/drivers/mtd/nand/cafe_nand.c
+++ b/drivers/mtd/nand/cafe_nand.c
@@ -303,13 +303,7 @@  static void cafe_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	case NAND_CMD_SEQIN:
 	case NAND_CMD_RNDIN:
 	case NAND_CMD_STATUS:
-	case NAND_CMD_DEPLETE1:
 	case NAND_CMD_RNDOUT:
-	case NAND_CMD_STATUS_ERROR:
-	case NAND_CMD_STATUS_ERROR0:
-	case NAND_CMD_STATUS_ERROR1:
-	case NAND_CMD_STATUS_ERROR2:
-	case NAND_CMD_STATUS_ERROR3:
 		cafe_writel(cafe, cafe->ctl2, NAND_CTRL2);
 		return;
 	}
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 0e28f55..760ee49 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -669,16 +669,6 @@  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_DEPLETE1:
-		return;
-
-	case NAND_CMD_STATUS_ERROR:
-	case NAND_CMD_STATUS_ERROR0:
-	case NAND_CMD_STATUS_ERROR1:
-	case NAND_CMD_STATUS_ERROR2:
-	case NAND_CMD_STATUS_ERROR3:
-		/* Read error status commands require only a short delay */
-		udelay(chip->chip_delay);
 		return;
 
 	case NAND_CMD_RESET:
diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index 891c52a..7199acc 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -218,7 +218,6 @@  MODULE_PARM_DESC(bch,		 "Enable BCH ecc and set how many bits should "
 #define STATE_CMD_READOOB      0x00000005 /* read OOB area */
 #define STATE_CMD_ERASE1       0x00000006 /* sector erase first command */
 #define STATE_CMD_STATUS       0x00000007 /* read status */
-#define STATE_CMD_STATUS_M     0x00000008 /* read multi-plane status (isn't implemented) */
 #define STATE_CMD_SEQIN        0x00000009 /* sequential data input */
 #define STATE_CMD_READID       0x0000000A /* read ID */
 #define STATE_CMD_ERASE2       0x0000000B /* sector erase second command */
@@ -406,8 +405,6 @@  static struct nandsim_operations {
 	{OPT_ANY, {STATE_CMD_ERASE1, STATE_ADDR_SEC, STATE_CMD_ERASE2 | ACTION_SECERASE, STATE_READY}},
 	/* Read status */
 	{OPT_ANY, {STATE_CMD_STATUS, STATE_DATAOUT_STATUS, STATE_READY}},
-	/* Read multi-plane status */
-	{OPT_SMARTMEDIA, {STATE_CMD_STATUS_M, STATE_DATAOUT_STATUS_M, STATE_READY}},
 	/* Read ID */
 	{OPT_ANY, {STATE_CMD_READID, STATE_ADDR_ZERO, STATE_DATAOUT_ID, STATE_READY}},
 	/* Large page devices read page */
@@ -1079,8 +1076,6 @@  static char *get_state_name(uint32_t state)
 			return "STATE_CMD_ERASE1";
 		case STATE_CMD_STATUS:
 			return "STATE_CMD_STATUS";
-		case STATE_CMD_STATUS_M:
-			return "STATE_CMD_STATUS_M";
 		case STATE_CMD_SEQIN:
 			return "STATE_CMD_SEQIN";
 		case STATE_CMD_READID:
@@ -1145,7 +1140,6 @@  static int check_command(int cmd)
 	case NAND_CMD_RNDOUTSTART:
 		return 0;
 
-	case NAND_CMD_STATUS_MULTI:
 	default:
 		return 1;
 	}
@@ -1171,8 +1165,6 @@  static uint32_t get_state_by_command(unsigned command)
 			return STATE_CMD_ERASE1;
 		case NAND_CMD_STATUS:
 			return STATE_CMD_STATUS;
-		case NAND_CMD_STATUS_MULTI:
-			return STATE_CMD_STATUS_M;
 		case NAND_CMD_SEQIN:
 			return STATE_CMD_SEQIN;
 		case NAND_CMD_READID:
diff --git a/drivers/mtd/nand/nuc900_nand.c b/drivers/mtd/nand/nuc900_nand.c
index a619119..cd6be2e 100644
--- a/drivers/mtd/nand/nuc900_nand.c
+++ b/drivers/mtd/nand/nuc900_nand.c
@@ -177,15 +177,6 @@  static void nuc900_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_DEPLETE1:
-		return;
-
-	case NAND_CMD_STATUS_ERROR:
-	case NAND_CMD_STATUS_ERROR0:
-	case NAND_CMD_STATUS_ERROR1:
-	case NAND_CMD_STATUS_ERROR2:
-	case NAND_CMD_STATUS_ERROR3:
-		udelay(chip->chip_delay);
 		return;
 
 	case NAND_CMD_RESET:
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 2aba049..69097e8 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -86,7 +86,6 @@  extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 #define NAND_CMD_READOOB	0x50
 #define NAND_CMD_ERASE1		0x60
 #define NAND_CMD_STATUS		0x70
-#define NAND_CMD_STATUS_MULTI	0x71
 #define NAND_CMD_SEQIN		0x80
 #define NAND_CMD_RNDIN		0x85
 #define NAND_CMD_READID		0x90
@@ -105,25 +104,6 @@  extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 #define NAND_CMD_RNDOUTSTART	0xE0
 #define NAND_CMD_CACHEDPROG	0x15
 
-/* Extended commands for AG-AND device */
-/*
- * Note: the command for NAND_CMD_DEPLETE1 is really 0x00 but
- *       there is no way to distinguish that from NAND_CMD_READ0
- *       until the remaining sequence of commands has been completed
- *       so add a high order bit and mask it off in the command.
- */
-#define NAND_CMD_DEPLETE1	0x100
-#define NAND_CMD_DEPLETE2	0x38
-#define NAND_CMD_STATUS_MULTI	0x71
-#define NAND_CMD_STATUS_ERROR	0x72
-/* multi-bank error status (banks 0-3) */
-#define NAND_CMD_STATUS_ERROR0	0x73
-#define NAND_CMD_STATUS_ERROR1	0x74
-#define NAND_CMD_STATUS_ERROR2	0x75
-#define NAND_CMD_STATUS_ERROR3	0x76
-#define NAND_CMD_STATUS_RESET	0x7f
-#define NAND_CMD_STATUS_CLEAR	0xff
-
 #define NAND_CMD_NONE		-1
 
 /* Status bits */