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

login
register
mail settings
Submitter Alexander Shiyan
Date Feb. 28, 2013, 8:02 a.m.
Message ID <1362038541-22406-3-git-send-email-shc_work@mail.ru>
Download mbox | patch
Permalink /patch/223804/
State New
Headers show

Comments

Alexander Shiyan - Feb. 28, 2013, 8:02 a.m.
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(-)
Brian Norris - March 5, 2013, 7:33 a.m.
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
Alexander Shiyan - March 5, 2013, 7:54 a.m.
> 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.

---

Patch

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
 }
 
 /**