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

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

[Alexander Shiyan]

NAND command, passed to cmd_ctrl(), is masked with 0xff. This patch
removes this since masking is not necessary and masking is not performed
in other places for same call.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mtd/nand/nand_base.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4321415..13c89c9 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -631,8 +631,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 	}
 
 	/* Command latch cycle */
-	chip->cmd_ctrl(mtd, command & 0xff,
-		       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
+	chip->cmd_ctrl(mtd, command, NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
 
 	if (column != -1 || page_addr != -1) {
 		int ctrl = NAND_CTRL_CHANGE | NAND_NCE | NAND_ALE;
-- 
1.7.3.4

[PATCH 2/3] mtd: nand_base: Removed unnecessary cleaning "onfi_version" variable

[Alexander Shiyan]

Variable "onfi_version" is already set to zero before nand_flash_detect_onfi()
call, so additional cleaning is not necessary.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mtd/nand/nand_base.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 13c89c9..382b857 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2888,8 +2888,6 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->onfi_version = 20;
 	else if (val & (1 << 1))
 		chip->onfi_version = 10;
-	else
-		chip->onfi_version = 0;
 
 	if (!chip->onfi_version) {
 		pr_info("%s: unsupported ONFI version: %d\n", __func__, val);
-- 
1.7.3.4

Re: [PATCH 2/3] mtd: nand_base: Removed unnecessary cleaning "onfi_version" variable

[Brian Norris]

On Thu, Feb 28, 2013 at 12:02 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> Variable "onfi_version" is already set to zero before nand_flash_detect_onfi()
> call, so additional cleaning is not necessary.
>
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Acked-by: Brian Norris <computersforpeace@gmail.com>

[PATCH 3/3] mtd: nand_base: Use readsb/readsw/writesb/writesw if possible

[Alexander Shiyan]

This patch provide using readsb/readsw/writesb/writesw functions
if it possible for particular platform.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mtd/nand/nand_base.c |   29 ++++++++++++++++++++++++-----
 1 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 382b857..4efde01 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -214,11 +214,16 @@ static void nand_select_chip(struct mtd_info *mtd, int chipnr)
  */
 static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
 {
-	int i;
 	struct nand_chip *chip = mtd->priv;
 
+#ifndef writesb
+	int i;
+
 	for (i = 0; i < len; i++)
 		writeb(buf[i], chip->IO_ADDR_W);
+#else
+	writesb(chip->IO_ADDR_W, buf, len);
+#endif
 }
 
 /**
@@ -231,11 +236,16 @@ static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
  */
 static void nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
 {
-	int i;
 	struct nand_chip *chip = mtd->priv;
 
+#ifndef readsb
+	int i;
+
 	for (i = 0; i < len; i++)
 		buf[i] = readb(chip->IO_ADDR_R);
+#else
+	readsb(chip->IO_ADDR_R, buf, len);
+#endif
 }
 
 /**
@@ -248,14 +258,18 @@ static void nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
  */
 static void nand_write_buf16(struct mtd_info *mtd, const uint8_t *buf, int len)
 {
-	int i;
 	struct nand_chip *chip = mtd->priv;
 	u16 *p = (u16 *) buf;
 	len >>= 1;
 
+#ifndef writesw
+	int i;
+
 	for (i = 0; i < len; i++)
 		writew(p[i], chip->IO_ADDR_W);
-
+#else
+	writesw(chip->IO_ADDR_W, p, len);
+#endif
 }
 
 /**
@@ -268,13 +282,18 @@ static void nand_write_buf16(struct mtd_info *mtd, const uint8_t *buf, int len)
  */
 static void nand_read_buf16(struct mtd_info *mtd, uint8_t *buf, int len)
 {
-	int i;
 	struct nand_chip *chip = mtd->priv;
 	u16 *p = (u16 *) buf;
 	len >>= 1;
 
+#ifndef readsw
+	int i;
+
 	for (i = 0; i < len; i++)
 		p[i] = readw(chip->IO_ADDR_R);
+#else
+	readsw(chip->IO_ADDR_R, p, len);
+#endif
 }
 
 /**
-- 
1.7.3.4

Re: [PATCH 3/3] mtd: nand_base: Use readsb/readsw/writesb/writesw if possible

[Brian Norris]

Hi Alexander,

I'm not an expert on IO accessors, but since you asked...

On 02/28/2013 12:02 AM, Alexander Shiyan wrote:
> This patch provide using readsb/readsw/writesb/writesw functions
> if it possible for particular platform.

The commit description does not give enough motivation. Should this 
improve performance? Or is this just a "cleanup"? (If the latter, then 
it fails, as the resulting code is much less clean.)

> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>   drivers/mtd/nand/nand_base.c |   29 ++++++++++++++++++++++++-----
>   1 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 382b857..4efde01 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -214,11 +214,16 @@ static void nand_select_chip(struct mtd_info *mtd, int chipnr)
>    */
>   static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
>   {
> -	int i;
>   	struct nand_chip *chip = mtd->priv;
>
> +#ifndef writesb

No one guarantees that 'writesb' will be a macro and not a static inline 
function, do they?

> +	int i;
> +
>   	for (i = 0; i < len; i++)
>   		writeb(buf[i], chip->IO_ADDR_W);
> +#else
> +	writesb(chip->IO_ADDR_W, buf, len);
> +#endif
>   }

These #ifndef's are very ugly and are likely the wrong way(TM). The 
first question I ask when I see this is: why isn't this interface 
available on all platforms? In fact, it seems like it has been dropped 
from the asm-generic headers and specifically recommended against. See:

commit b2656a138ab7bc4e7abd3b1cbd6d1f105c7a7186
Author: Will Deacon <will.deacon@arm.com>
Date:   Wed Oct 17 16:45:01 2012 +0100

     asm-generic: io: remove {read,write} string functions

Quote: "We should encourage driver writers to use the 
io{read,write}{8,16,32}_rep functions instead."

So, an alternative patch might use iowrite8_rep() here unconditionally.

>   /**

<snipped the rest; same comments apply>

Brian

Re[2]: [PATCH 3/3] mtd: nand_base: Use readsb/readsw/writesb/writesw if possible

[Alexander Shiyan]

> Hi Alexander,
> 
> I'm not an expert on IO accessors, but since you asked...
> 
> On 02/28/2013 12:02 AM, Alexander Shiyan wrote:
> > This patch provide using readsb/readsw/writesb/writesw functions
> > if it possible for particular platform.
> 
> The commit description does not give enough motivation. Should this 
> improve performance? Or is this just a "cleanup"? (If the latter, then 
> it fails, as the resulting code is much less clean.)

Basic idea is a performance, because architecture may have a
improved functions for multiple read/write.

> 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >   drivers/mtd/nand/nand_base.c |   29 ++++++++++++++++++++++++-----
> >   1 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 382b857..4efde01 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -214,11 +214,16 @@ static void nand_select_chip(struct mtd_info *mtd, int chipnr)
> >    */
> >   static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
> >   {
> > -	int i;
> >   	struct nand_chip *chip = mtd->priv;
> >
> > +#ifndef writesb
> 
> No one guarantees that 'writesb' will be a macro and not a static inline 
> function, do they?
> 
> > +	int i;
> > +
> >   	for (i = 0; i < len; i++)
> >   		writeb(buf[i], chip->IO_ADDR_W);
> > +#else
> > +	writesb(chip->IO_ADDR_W, buf, len);
> > +#endif
> >   }
> 
> These #ifndef's are very ugly and are likely the wrong way(TM). The 
> first question I ask when I see this is: why isn't this interface 
> available on all platforms? In fact, it seems like it has been dropped 
> from the asm-generic headers and specifically recommended against. See:
> 
> commit b2656a138ab7bc4e7abd3b1cbd6d1f105c7a7186
> Author: Will Deacon <will.deacon@arm.com>
> Date:   Wed Oct 17 16:45:01 2012 +0100
> 
>      asm-generic: io: remove {read,write} string functions
> 
> Quote: "We should encourage driver writers to use the 
> io{read,write}{8,16,32}_rep functions instead."
> 
> So, an alternative patch might use iowrite8_rep() here unconditionally.

Indeed. So I can remade patch to use ioread/iowrite and remove
unnecessary #ifdefs if you have not additional comments.

---

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

[Brian Norris]

On Thu, Feb 28, 2013 at 12:02 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> NAND command, passed to cmd_ctrl(), is masked with 0xff. This patch
> removes this since masking is not necessary and masking is not performed
> in other places for same call.

As I commented on the other thread, this is not a sufficient
explanation. AG-AND (soon to be removed) may use a fake 9-bit command
which could expect this masking, even though masking is not done
everywhere. Please document why the command does not need to be
masked.

Thanks,
Brian

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

[Alexander Shiyan]

> On Thu, Feb 28, 2013 at 12:02 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> > NAND command, passed to cmd_ctrl(), is masked with 0xff. This patch
> > removes this since masking is not necessary and masking is not performed
> > in other places for same call.
> 
> As I commented on the other thread, this is not a sufficient
> explanation. AG-AND (soon to be removed) may use a fake 9-bit command
> which could expect this masking, even though masking is not done
> everywhere. Please document why the command does not need to be
> masked.

I think the best if Artem take this part into his patch.
And what about [3/3] part?

---

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

[Artem Bityutskiy]

On Thu, 2013-02-28 at 12:02 +0400, Alexander Shiyan wrote:
> NAND command, passed to cmd_ctrl(), is masked with 0xff. This patch
> removes this since masking is not necessary and masking is not performed
> in other places for same call.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Pushed patches 1 and 2 to l2-mtd.git. Did not take patch 3. Thanks!

-- 
Best Regards,
Artem Bityutskiy