Patchwork [1/2] mtd mxc_nand: use 32bit copy functions

login
register
mail settings
Submitter Sascha Hauer
Date May 25, 2012, 2:22 p.m.
Message ID <1337955762-19157-1-git-send-email-s.hauer@pengutronix.de>
Download mbox | patch
Permalink /patch/161371/
State New
Headers show

Comments

Sascha Hauer - May 25, 2012, 2:22 p.m.
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>
---
 drivers/mtd/nand/mxc_nand.c |   36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)
Artem Bityutskiy - May 25, 2012, 2:58 p.m.
On Fri, 2012-05-25 at 16:22 +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.

Could you please take a look at the checkpatch.pl updates?

WARNING:LONG_LINE: line over 80 characters
#103: FILE: drivers/mtd/nand/mxc_nand.c:276:
+static void memcpy32_fromio(void *trg, const volatile void __iomem  *src, size_t size)

WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#103: FILE: drivers/mtd/nand/mxc_nand.c:276:
+static void memcpy32_fromio(void *trg, const volatile void __iomem  *src, size_t size)

WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#107: FILE: drivers/mtd/nand/mxc_nand.c:280:
+       const volatile u32 *s = src;

WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#113: FILE: drivers/mtd/nand/mxc_nand.c:286:
+static void memcpy32_toio(volatile void __iomem *trg, const void *src, int size)

WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#116: FILE: drivers/mtd/nand/mxc_nand.c:289:
+       volatile u32 *t = trg;

WARNING:LONG_LINE: line over 80 characters
#170: FILE: drivers/mtd/nand/mxc_nand.c:1093:
+               memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
Sascha Hauer - May 25, 2012, 2:59 p.m.
On Fri, May 25, 2012 at 05:58:22PM +0300, Artem Bityutskiy wrote:
> On Fri, 2012-05-25 at 16:22 +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.
> 
> Could you please take a look at the checkpatch.pl updates?

Will do.

> 
> WARNING:LONG_LINE: line over 80 characters
> #103: FILE: drivers/mtd/nand/mxc_nand.c:276:
> +static void memcpy32_fromio(void *trg, const volatile void __iomem  *src, size_t size)
> 
> WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> #103: FILE: drivers/mtd/nand/mxc_nand.c:276:
> +static void memcpy32_fromio(void *trg, const volatile void __iomem  *src, size_t size)

This makes me wonder a bit, I basically copied the prototype from
the _memcpy_*_io template from arch/arm/kernel/io.c. Should they
be wrong?
otoh I also wondered why there were volatiles in arch/arm/kernel/io.c
in the first place ;)

Sascha
Uwe Kleine-König - June 27, 2012, 5:52 p.m.
Hello,

On Fri, May 25, 2012 at 04:22:41PM +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.
> 
> 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>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I don't know about the sparse problems this introduces, but without this
patch the mxc-nand driver is broken, so it should really go in before
3.5.

Best regards
Uwe
Artem Bityutskiy - June 29, 2012, 11:25 a.m.
On Wed, 2012-06-27 at 19:52 +0200, Uwe Kleine-König wrote:
> I don't know about the sparse problems this introduces, but without this
> patch the mxc-nand driver is broken, so it should really go in before
> 3.5.

The problems, AFAIU, are that you cannot directly dereference __iomem
pointers, you always need to use helpers like 'readl()'. This patch
introduces the following warnings:

+drivers/mtd/nand/mxc_nand.c:281:33: warning: incorrect type in initializer (different address spaces) [sparse]
+drivers/mtd/nand/mxc_nand.c:281:33:    expected unsigned int const volatile [usertype] *s [sparse]
+drivers/mtd/nand/mxc_nand.c:281:33:    got void const volatile [noderef] <asn:2>*src [sparse]
+drivers/mtd/nand/mxc_nand.c:284 memcpy32_fromio() warn: side effect in macro '__raw_readl' doing 's++' [smatch]
+drivers/mtd/nand/mxc_nand.c:284:24: warning: incorrect type in argument 1 (different address spaces) [sparse]
+drivers/mtd/nand/mxc_nand.c:284:24:    expected void const volatile [noderef] <asn:2>*<noident> [sparse]
+drivers/mtd/nand/mxc_nand.c:284:24:    got unsigned int const volatile [usertype] * [sparse]
+drivers/mtd/nand/mxc_nand.c:290:27: warning: incorrect type in initializer (different address spaces) [sparse]
+drivers/mtd/nand/mxc_nand.c:290:27:    expected unsigned int volatile [usertype] *t [sparse]
+drivers/mtd/nand/mxc_nand.c:290:27:    got void volatile [noderef] <asn:2>*trg [sparse]
+drivers/mtd/nand/mxc_nand.c:294 memcpy32_toio() warn: side effect in macro '__raw_writel' doing 't++' [smatch]
+drivers/mtd/nand/mxc_nand.c:294:17: warning: incorrect type in argument 1 (different address spaces) [sparse]
+drivers/mtd/nand/mxc_nand.c:294:17:    expected void const volatile [noderef] <asn:2>*<noident> [sparse]
+drivers/mtd/nand/mxc_nand.c:294:17:    got unsigned int volatile [usertype] * [sparse]

The "volitile" should be also removed.
Uwe Kleine-König - June 29, 2012, 11:28 a.m.
Hi Artem,

On Fri, Jun 29, 2012 at 02:25:24PM +0300, Artem Bityutskiy wrote:
> On Wed, 2012-06-27 at 19:52 +0200, Uwe Kleine-König wrote:
> > I don't know about the sparse problems this introduces, but without this
> > patch the mxc-nand driver is broken, so it should really go in before
> > 3.5.
> 
> The problems, AFAIU, are that you cannot directly dereference __iomem
> pointers, you always need to use helpers like 'readl()'. This patch
> introduces the following warnings:
> 
> +drivers/mtd/nand/mxc_nand.c:281:33: warning: incorrect type in initializer (different address spaces) [sparse]
> +drivers/mtd/nand/mxc_nand.c:281:33:    expected unsigned int const volatile [usertype] *s [sparse]
> +drivers/mtd/nand/mxc_nand.c:281:33:    got void const volatile [noderef] <asn:2>*src [sparse]
> +drivers/mtd/nand/mxc_nand.c:284 memcpy32_fromio() warn: side effect in macro '__raw_readl' doing 's++' [smatch]
> +drivers/mtd/nand/mxc_nand.c:284:24: warning: incorrect type in argument 1 (different address spaces) [sparse]
> +drivers/mtd/nand/mxc_nand.c:284:24:    expected void const volatile [noderef] <asn:2>*<noident> [sparse]
> +drivers/mtd/nand/mxc_nand.c:284:24:    got unsigned int const volatile [usertype] * [sparse]
> +drivers/mtd/nand/mxc_nand.c:290:27: warning: incorrect type in initializer (different address spaces) [sparse]
> +drivers/mtd/nand/mxc_nand.c:290:27:    expected unsigned int volatile [usertype] *t [sparse]
> +drivers/mtd/nand/mxc_nand.c:290:27:    got void volatile [noderef] <asn:2>*trg [sparse]
> +drivers/mtd/nand/mxc_nand.c:294 memcpy32_toio() warn: side effect in macro '__raw_writel' doing 't++' [smatch]
> +drivers/mtd/nand/mxc_nand.c:294:17: warning: incorrect type in argument 1 (different address spaces) [sparse]
> +drivers/mtd/nand/mxc_nand.c:294:17:    expected void const volatile [noderef] <asn:2>*<noident> [sparse]
> +drivers/mtd/nand/mxc_nand.c:294:17:    got unsigned int volatile [usertype] * [sparse]
> 
> The "volitile" should be also removed.
Did you see that Sascha posted a v2?

Best regards
Uwe
Artem Bityutskiy - June 29, 2012, 11:34 a.m.
On Fri, 2012-05-25 at 16:59 +0200, Sascha Hauer wrote:
> > WARNING:LONG_LINE: line over 80 characters
> > #103: FILE: drivers/mtd/nand/mxc_nand.c:276:
> > +static void memcpy32_fromio(void *trg, const volatile void __iomem  *src, size_t size)
> > 
> > WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> > #103: FILE: drivers/mtd/nand/mxc_nand.c:276:
> > +static void memcpy32_fromio(void *trg, const volatile void __iomem  *src, size_t size)
> 
> This makes me wonder a bit, I basically copied the prototype from
> the _memcpy_*_io template from arch/arm/kernel/io.c. Should they
> be wrong?

Well, this is arch-specific code. In there we may make various
assumptions about CPU not doing re-ordering. You are changing generic
code, you should not do assumptions like this.

How about adding these functions to arch/arm/kernel/io.c instead?

> otoh I also wondered why there were volatiles in arch/arm/kernel/io.c
> in the first place ;)

Not sure, probably in that file we assume that the memory is
strongly-ordered.

Patch

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index fd14966..4d27ddc 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 volatile void __iomem  *src, size_t size)
+{
+	int i;
+	u32 *t = trg;
+	const volatile u32 *s = src;
+
+	for (i = 0; i < (size >> 2); i++)
+		*t++ = __raw_readl(s++);
+}
+
+static void memcpy32_toio(volatile void __iomem *trg, const void *src, int size)
+{
+	int i;
+	volatile u32 *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,7 @@  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 +1106,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);