[PATCH v2] mtd mxc_nand: use 32bit copy functions

[PATCH v2] mtd mxc_nand: use 32bit copy functions

[Sascha Hauer]

The following commit changes the function used to copy from/to
the hardware buffer to memcpy_[from|to]io. This does not work
since the hardware cannot handle the byte accesses used by these
functions. Instead of reverting this patch introduce 32bit
correspondents of these functions.

| commit 5775ba36ea9c760c2d7e697dac04f2f7fc95aa62
| Author: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
| Date:   Tue Apr 24 10:05:22 2012 +0200
|
|    mtd: mxc_nand: fix several sparse warnings about incorrect address space
|
|     Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
|     Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---

v2: remove volatile, fix line over 80 characters

 drivers/mtd/nand/mxc_nand.c |   37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index fd14966..17ddcf0 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -273,6 +273,26 @@ static struct nand_ecclayout nandv2_hw_eccoob_4k = {
 
 static const char *part_probes[] = { "RedBoot", "cmdlinepart", "ofpart", NULL };
 
+static void memcpy32_fromio(void *trg, const void __iomem  *src, size_t size)
+{
+	int i;
+	u32 *t = trg;
+	const __iomem u32 *s = src;
+
+	for (i = 0; i < (size >> 2); i++)
+		*t++ = __raw_readl(s++);
+}
+
+static void memcpy32_toio(void __iomem *trg, const void *src, int size)
+{
+	int i;
+	u32 __iomem *t = trg;
+	const u32 *s = src;
+
+	for (i = 0; i < (size >> 2); i++)
+		__raw_writel(*s++, t++);
+}
+
 static int check_int_v3(struct mxc_nand_host *host)
 {
 	uint32_t tmp;
@@ -519,7 +539,7 @@ static void send_read_id_v3(struct mxc_nand_host *host)
 
 	wait_op_done(host, true);
 
-	memcpy_fromio(host->data_buf, host->main_area0, 16);
+	memcpy32_fromio(host->data_buf, host->main_area0, 16);
 }
 
 /* Request the NANDFC to perform a read of the NAND device ID. */
@@ -535,7 +555,7 @@ static void send_read_id_v1_v2(struct mxc_nand_host *host)
 	/* Wait for operation to complete */
 	wait_op_done(host, true);
 
-	memcpy_fromio(host->data_buf, host->main_area0, 16);
+	memcpy32_fromio(host->data_buf, host->main_area0, 16);
 
 	if (this->options & NAND_BUSWIDTH_16) {
 		/* compress the ID info */
@@ -797,16 +817,16 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
 
 	if (bfrom) {
 		for (i = 0; i < n - 1; i++)
-			memcpy_fromio(d + i * j, s + i * t, j);
+			memcpy32_fromio(d + i * j, s + i * t, j);
 
 		/* the last section */
-		memcpy_fromio(d + i * j, s + i * t, mtd->oobsize - i * j);
+		memcpy32_fromio(d + i * j, s + i * t, mtd->oobsize - i * j);
 	} else {
 		for (i = 0; i < n - 1; i++)
-			memcpy_toio(&s[i * t], &d[i * j], j);
+			memcpy32_toio(&s[i * t], &d[i * j], j);
 
 		/* the last section */
-		memcpy_toio(&s[i * t], &d[i * j], mtd->oobsize - i * j);
+		memcpy32_toio(&s[i * t], &d[i * j], mtd->oobsize - i * j);
 	}
 }
 
@@ -1070,7 +1090,8 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 
 		host->devtype_data->send_page(mtd, NFC_OUTPUT);
 
-		memcpy_fromio(host->data_buf, host->main_area0, mtd->writesize);
+		memcpy32_fromio(host->data_buf, host->main_area0,
+				mtd->writesize);
 		copy_spare(mtd, true);
 		break;
 
@@ -1086,7 +1107,7 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		break;
 
 	case NAND_CMD_PAGEPROG:
-		memcpy_toio(host->main_area0, host->data_buf, mtd->writesize);
+		memcpy32_toio(host->main_area0, host->data_buf, mtd->writesize);
 		copy_spare(mtd, false);
 		host->devtype_data->send_page(mtd, NFC_INPUT);
 		host->devtype_data->send_cmd(host, command, true);
-- 
1.7.10

[PATCH v2] mtd mxc_nand: use 32bit copy functions

[Uwe Kleine-König]

Hello,

On Tue, May 29, 2012 at 10:16:09AM +0200, Sascha Hauer wrote:
> The following commit changes the function used to copy from/to
> the hardware buffer to memcpy_[from|to]io. This does not work
> since the hardware cannot handle the byte accesses used by these
> functions. Instead of reverting this patch introduce 32bit
> correspondents of these functions.
Hmm, I didn't run an mtd test suite, but on mx27 it worked for me. IMHO
it's surprising that memcpy used to work, but memcpy_fromio doesn't. I
wouldn't expect a different semantic (apart from normal vs. __iomem
memory). And I wonder what will break when ARM's memcpy_fromio et al.
is optimized.

> | commit 5775ba36ea9c760c2d7e697dac04f2f7fc95aa62
> | Author: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> | Date:   Tue Apr 24 10:05:22 2012 +0200
> |
> |    mtd: mxc_nand: fix several sparse warnings about incorrect address space
> |
> |     Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> |     Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> 
> v2: remove volatile, fix line over 80 characters
I'm not sure about the need for volatile. I don't have my C Reference
handy, but maybe adding volatile prevents the compilier issuing code
that e.g. uses a different word size?! Not sure if it matters for a
static and so maybe inlined function.

Still:
Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

> 
>  drivers/mtd/nand/mxc_nand.c |   37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index fd14966..17ddcf0 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -273,6 +273,26 @@ static struct nand_ecclayout nandv2_hw_eccoob_4k = {
>  
>  static const char *part_probes[] = { "RedBoot", "cmdlinepart", "ofpart", NULL };
>  
> +static void memcpy32_fromio(void *trg, const void __iomem  *src, size_t size)
> +{
> +	int i;
> +	u32 *t = trg;
> +	const __iomem u32 *s = src;
> +
> +	for (i = 0; i < (size >> 2); i++)
> +		*t++ = __raw_readl(s++);
> +}
> +
> +static void memcpy32_toio(void __iomem *trg, const void *src, int size)
> +{
> +	int i;
> +	u32 __iomem *t = trg;
> +	const u32 *s = src;
> +
> +	for (i = 0; i < (size >> 2); i++)
> +		__raw_writel(*s++, t++);
> +}
> +
>  static int check_int_v3(struct mxc_nand_host *host)
>  {
>  	uint32_t tmp;
> @@ -519,7 +539,7 @@ static void send_read_id_v3(struct mxc_nand_host *host)
>  
>  	wait_op_done(host, true);
>  
> -	memcpy_fromio(host->data_buf, host->main_area0, 16);
> +	memcpy32_fromio(host->data_buf, host->main_area0, 16);
>  }
>  
>  /* Request the NANDFC to perform a read of the NAND device ID. */
> @@ -535,7 +555,7 @@ static void send_read_id_v1_v2(struct mxc_nand_host *host)
>  	/* Wait for operation to complete */
>  	wait_op_done(host, true);
>  
> -	memcpy_fromio(host->data_buf, host->main_area0, 16);
> +	memcpy32_fromio(host->data_buf, host->main_area0, 16);
>  
>  	if (this->options & NAND_BUSWIDTH_16) {
>  		/* compress the ID info */
> @@ -797,16 +817,16 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
>  
>  	if (bfrom) {
>  		for (i = 0; i < n - 1; i++)
> -			memcpy_fromio(d + i * j, s + i * t, j);
> +			memcpy32_fromio(d + i * j, s + i * t, j);
>  
>  		/* the last section */
> -		memcpy_fromio(d + i * j, s + i * t, mtd->oobsize - i * j);
> +		memcpy32_fromio(d + i * j, s + i * t, mtd->oobsize - i * j);
>  	} else {
>  		for (i = 0; i < n - 1; i++)
> -			memcpy_toio(&s[i * t], &d[i * j], j);
> +			memcpy32_toio(&s[i * t], &d[i * j], j);
>  
>  		/* the last section */
> -		memcpy_toio(&s[i * t], &d[i * j], mtd->oobsize - i * j);
> +		memcpy32_toio(&s[i * t], &d[i * j], mtd->oobsize - i * j);
>  	}
>  }
>  
> @@ -1070,7 +1090,8 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
>  
>  		host->devtype_data->send_page(mtd, NFC_OUTPUT);
>  
> -		memcpy_fromio(host->data_buf, host->main_area0, mtd->writesize);
> +		memcpy32_fromio(host->data_buf, host->main_area0,
> +				mtd->writesize);
>  		copy_spare(mtd, true);
>  		break;
>  
> @@ -1086,7 +1107,7 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
>  		break;
>  
>  	case NAND_CMD_PAGEPROG:
> -		memcpy_toio(host->main_area0, host->data_buf, mtd->writesize);
> +		memcpy32_toio(host->main_area0, host->data_buf, mtd->writesize);
>  		copy_spare(mtd, false);
>  		host->devtype_data->send_page(mtd, NFC_INPUT);
>  		host->devtype_data->send_cmd(host, command, true);
> -- 
> 1.7.10
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2] mtd mxc_nand: use 32bit copy functions

[Sascha Hauer]

On Tue, May 29, 2012 at 11:12:54AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Tue, May 29, 2012 at 10:16:09AM +0200, Sascha Hauer wrote:
> > The following commit changes the function used to copy from/to
> > the hardware buffer to memcpy_[from|to]io. This does not work
> > since the hardware cannot handle the byte accesses used by these
> > functions. Instead of reverting this patch introduce 32bit
> > correspondents of these functions.
> Hmm, I didn't run an mtd test suite, but on mx27 it worked for me. IMHO
> it's surprising that memcpy used to work, but memcpy_fromio doesn't. I
> wouldn't expect a different semantic (apart from normal vs. __iomem
> memory). And I wonder what will break when ARM's memcpy_fromio et al.
> is optimized.

Have a look at the (ARM) implementation of memcpy_fromio:

void _memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
{
	unsigned char *t = to;
	while (count) {
		count--;
		*t = readb(from);
		t++;
		from++;
	}
}

Appearently this uses byte accesses which do not work on NFC SRAM,
whereas memcpy uses optimized (so 32bit whenever possible) accesses.
If someone would implement an optimized version of memcpy_fromio, it
would work for the NFC aswell.

btw on i.MX27 byte accesses also do not work on NFC SRAM, so I doubt
this worked for you.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2] mtd mxc_nand: use 32bit copy functions

[Russell King - ARM Linux]

On Tue, May 29, 2012 at 11:36:29AM +0200, Sascha Hauer wrote:
> On Tue, May 29, 2012 at 11:12:54AM +0200, Uwe Kleine-K?nig wrote:
> > Hello,
> > 
> > On Tue, May 29, 2012 at 10:16:09AM +0200, Sascha Hauer wrote:
> > > The following commit changes the function used to copy from/to
> > > the hardware buffer to memcpy_[from|to]io. This does not work
> > > since the hardware cannot handle the byte accesses used by these
> > > functions. Instead of reverting this patch introduce 32bit
> > > correspondents of these functions.
> > Hmm, I didn't run an mtd test suite, but on mx27 it worked for me. IMHO
> > it's surprising that memcpy used to work, but memcpy_fromio doesn't. I
> > wouldn't expect a different semantic (apart from normal vs. __iomem
> > memory). And I wonder what will break when ARM's memcpy_fromio et al.
> > is optimized.
> 
> Have a look at the (ARM) implementation of memcpy_fromio:
> 
> void _memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
> {
> 	unsigned char *t = to;
> 	while (count) {
> 		count--;
> 		*t = readb(from);
> 		t++;
> 		from++;
> 	}
> }
> 
> Appearently this uses byte accesses which do not work on NFC SRAM,
> whereas memcpy uses optimized (so 32bit whenever possible) accesses.
> If someone would implement an optimized version of memcpy_fromio, it
> would work for the NFC aswell.
> 
> btw on i.MX27 byte accesses also do not work on NFC SRAM, so I doubt
> this worked for you.

And then when you ask for an odd alignment or an odd number of bytes...

[PATCH v2] mtd mxc_nand: use 32bit copy functions

[Sascha Hauer]

On Tue, May 29, 2012 at 10:39:48AM +0100, Russell King - ARM Linux wrote:
> On Tue, May 29, 2012 at 11:36:29AM +0200, Sascha Hauer wrote:
> > On Tue, May 29, 2012 at 11:12:54AM +0200, Uwe Kleine-K?nig wrote:
> > > Hello,
> > > 
> > > On Tue, May 29, 2012 at 10:16:09AM +0200, Sascha Hauer wrote:
> > > > The following commit changes the function used to copy from/to
> > > > the hardware buffer to memcpy_[from|to]io. This does not work
> > > > since the hardware cannot handle the byte accesses used by these
> > > > functions. Instead of reverting this patch introduce 32bit
> > > > correspondents of these functions.
> > > Hmm, I didn't run an mtd test suite, but on mx27 it worked for me. IMHO
> > > it's surprising that memcpy used to work, but memcpy_fromio doesn't. I
> > > wouldn't expect a different semantic (apart from normal vs. __iomem
> > > memory). And I wonder what will break when ARM's memcpy_fromio et al.
> > > is optimized.
> > 
> > Have a look at the (ARM) implementation of memcpy_fromio:
> > 
> > void _memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
> > {
> > 	unsigned char *t = to;
> > 	while (count) {
> > 		count--;
> > 		*t = readb(from);
> > 		t++;
> > 		from++;
> > 	}
> > }
> > 
> > Appearently this uses byte accesses which do not work on NFC SRAM,
> > whereas memcpy uses optimized (so 32bit whenever possible) accesses.
> > If someone would implement an optimized version of memcpy_fromio, it
> > would work for the NFC aswell.
> > 
> > btw on i.MX27 byte accesses also do not work on NFC SRAM, so I doubt
> > this worked for you.
> 
> And then when you ask for an odd alignment or an odd number of bytes...

Then the person will get an oops and hopefully report it, which so far
did not happen. I could try and implement this, but I rather don't do
this as long as we don't know what it's used for and thus we can check
that the result is correct.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2] mtd mxc_nand: use 32bit copy functions

[Artem Bityutskiy]

On Tue, 2012-05-29 at 10:16 +0200, Sascha Hauer wrote:
> The following commit changes the function used to copy from/to
> the hardware buffer to memcpy_[from|to]io. This does not work
> since the hardware cannot handle the byte accesses used by these
> functions. Instead of reverting this patch introduce 32bit
> correspondents of these functions.

Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120629/f0d9ca4a/attachment-0001.sig>

mxc-nand fix for 3.5 (Was: Re: [PATCH v2] mtd mxc_nand: use 32bit copy functions)

[Uwe Kleine-König]

On Fri, Jun 29, 2012 at 02:46:09PM +0300, Artem Bityutskiy wrote:
> On Tue, 2012-05-29 at 10:16 +0200, Sascha Hauer wrote:
> > The following commit changes the function used to copy from/to
> > the hardware buffer to memcpy_[from|to]io. This does not work
> > since the hardware cannot handle the byte accesses used by these
> > functions. Instead of reverting this patch introduce 32bit
> > correspondents of these functions.
> 
> Pushed to l2-mtd.git, thanks!
The mxc_nand driver is completely useless with

	5775ba3 (mtd: mxc_nand: fix several sparse warnings about incorrect address space)

but without this patch. So it would be great if we could have it in 3.5.
Or alternatively revert 5775ba3. This fix currently it sits as

	096bcc2 (mtd: mxc_nand: use 32bit copy functions)

in next/master.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

mxc-nand fix for 3.5 (Was: Re: [PATCH v2] mtd mxc_nand: use 32bit copy functions)

[Woodhouse, David]

On Thu, 2012-07-12 at 08:57 +0200, Uwe Kleine-K?nig wrote:
> The mxc_nand driver is completely useless with
> 
>         5775ba3 (mtd: mxc_nand: fix several sparse warnings about incorrect address space)
> 
> but without this patch. So it would be great if we could have it in 3.5.
> Or alternatively revert 5775ba3. This fix currently it sits as
> 
>         096bcc2 (mtd: mxc_nand: use 32bit copy functions)
> 
> in next/master. 

The subject of that commit didn't jump out at me when I was perusing the
patch queue for 3.5-worthy stuff.

Artem reminded me about it just after I sent my previous pull request to
Linus last week. I wanted to let it sit in linux-next for a little
while, but now...

Linus, please pull from
   git://git.infradead.org/linux-mtd.git for-linus-20120712

Late MTD fixes for 3.5:
 - fix 'sparse warning fix' regression which totally breaks MXC NAND
 - fix GPMI NAND regression when used with UBI
 - update/correct sysfs documentation for new 'bitflip_threshold' field
 - fix nandsim build failure

----
Herton Ronaldo Krzesinski (1):
      mtd: nandsim: don't open code a do_div helper

Mike Dunn (1):
      mtd: ABI documentation: clarification of bitflip_threshold

Sascha Hauer (2):
      mtd: mxc_nand: use 32bit copy functions
      mtd: gpmi-nand: fix read page when reading to vmalloced area

 Documentation/ABI/testing/sysfs-class-mtd |   17 ++++++-------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c    |   10 ++++----
 drivers/mtd/nand/mxc_nand.c               |   37 ++++++++++++++++++++++-------
 drivers/mtd/nand/nandsim.c                |   12 +++-------
 4 files changed, 46 insertions(+), 30 deletions(-)

-- 
                   Sent with MeeGo's ActiveSync support.

David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation


-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 4370 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120712/7d3ca2c4/attachment-0001.bin>