mtd mxc_nand: use 32bit copy functions

PATCH Superseded Sascha Hauer 4 participants 10 messages May 25, 2012 – Jun 29, 2012

Subsystems (3)

Arm i.MX and Layerscape Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Sascha Hauer

author maintainer

2012-05-25 14:22 UTC #0 lore raw

PATCH 1/2 mtd mxc_nand: use 32bit copy functions

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 <[email protected]>
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 <[email protected]>

Signed-off-by Artem Bityutskiy <[email protected]>

Signed-off-by Sascha Hauer <[email protected]>

Cc Uwe Kleine-K?nig <[email protected]>

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

drivers/mtd/nand/mxc_nand.c +28 -8

index fd14966..4d27ddc 100644
@@ -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);

Signature · 1 line

1.7.10

Sascha Hauer

author maintainer

2012-05-25 14:22 UTC #1 lore raw

PATCH 2/2 mtd mxc_nand: move ecc strengh setup before nand_scan_tail

Since this commit:

commit 6a918bade9dab40aaef80559bd1169c69e8d69cb
Author: Mike Dunn <[email protected]>
Date: Sun Mar 11 14:21:11 2012 -0700

mtd: flash drivers set ecc strength

The mxc_nand driver fails with:

Driver must set ecc.strength when using hardware ECC

This is because nand_scan_tail checks for correct ecc strength settings, so we must set them up before nand_scan_tail.

Signed-off-by Sascha Hauer <[email protected]>

Cc stable at vger.kernel.org

---
 drivers/mtd/nand/mxc_nand.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/mtd/nand/mxc_nand.c +6 -6

index 4d27ddc..aaf042b 100644
@@ -1485,12 +1485,6 @@ static int __init mxcnd_probe(struct platform_device *pdev)
 	else if (mtd->writesize == 4096)
 		this->ecc.layout = host->devtype_data->ecclayout_4k;
 
-	/* second phase scan */
-	if (nand_scan_tail(mtd)) {
-		err = -ENXIO;
-		goto escan;
-	}
-
 	if (this->ecc.mode == NAND_ECC_HW) {
 		if (nfc_is_v1())
 			this->ecc.strength = 1;

@@ -1498,6 +1492,12 @@ static int __init mxcnd_probe(struct platform_device *pdev)
 			this->ecc.strength = (host->eccsize == 4) ? 4 : 8;
 	}
 
+	/* second phase scan */
+	if (nand_scan_tail(mtd)) {
+		err = -ENXIO;
+		goto escan;
+	}
+
 	/* Register the partitions */
 	mtd_device_parse_register(mtd, part_probes,
 			&(struct mtd_part_parser_data){

Signature · 1 line

1.7.10

Mike Dunn

2012-05-25 16:29 UTC #2 lore raw

PATCH 2/2 mtd mxc_nand: move ecc strengh setup before nand_scan_tail

Quoted 6 lines · Sascha Hauer

Since this commit:
commit 6a918bade9dab40aaef80559bd1169c69e8d69cb
Author: Mike Dunn <[email protected]>
Date: Sun Mar 11 14:21:11 2012 -0700

Quoted 8 lines

mtd: flash drivers set ecc strength
The mxc_nand driver fails with:
Driver must set ecc.strength when using hardware ECC
This is because nand_scan_tail checks for correct ecc strength settings, so we must set them up before nand_scan_tail.

Oops, sorry Sascha. This was compile-tested only, but I should have ensured strength was set before call to nand_scan_tail().

Thanks,
Mike

Artem Bityutskiy

2012-05-25 16:55 UTC #3 lore raw

PATCH 2/2 mtd mxc_nand: move ecc strengh setup before nand_scan_tail

In reply to Sascha Hauer

Quoted 6 lines · Sascha Hauer

Since this commit:
commit 6a918bade9dab40aaef80559bd1169c69e8d69cb
Author: Mike Dunn <[email protected]>
Date: Sun Mar 11 14:21:11 2012 -0700

Quoted 8 lines

mtd: flash drivers set ecc strength
The mxc_nand driver fails with:
Driver must set ecc.strength when using hardware ECC
This is because nand_scan_tail checks for correct ecc strength settings, so we must set them up before nand_scan_tail.

Pushed this one to l2-mtd.git as well, thanks!

Signature · 9 lines

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/20120525/5227da8b/attachment-0001.sig>

Artem Bityutskiy

2012-05-25 14:58 UTC #4 lore raw

PATCH 1/2 mtd mxc_nand: use 32bit copy functions

In reply to Sascha Hauer

Quoted 5 lines · 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.

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);

Signature · 9 lines

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/20120525/e753d664/attachment.sig>

Sascha Hauer

author maintainer

2012-05-25 14:59 UTC #5 lore raw

PATCH 1/2 mtd mxc_nand: use 32bit copy functions

Quoted 8 lines · Artem Bityutskiy

Quoted 5 lines · 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.
Could you please take a look at the checkpatch.pl updates?

Will do.

Quoted 8 lines

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

Signature · 4 lines

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 |

Artem Bityutskiy

2012-06-29 11:34 UTC #6 lore raw

PATCH 1/2 mtd mxc_nand: use 32bit copy functions

Quoted 11 lines · Sascha Hauer

Quoted 7 lines
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?

Quoted 2 lines

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.

Signature · 9 lines

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/942ccb1a/attachment.sig>

Uwe Kleine-König

2012-06-27 17:52 UTC #7 lore raw

PATCH 1/2 mtd mxc_nand: use 32bit copy functions

In reply to Sascha Hauer

Hello,

Quoted 10 lines · 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 <[email protected]>
Date:   Tue Apr 24 10:05:22 2012 +0200

Quoted 7 lines

mtd: mxc_nand: fix several sparse warnings about incorrect address space
Signed-off-by Uwe Kleine-K?nig <[email protected]>
Signed-off-by Artem Bityutskiy <[email protected]>
Signed-off-by Sascha Hauer <[email protected]>
Cc Uwe Kleine-K?nig <[email protected]>

Acked-by Uwe Kleine-K?nig <[email protected]>

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

Signature · 2 lines

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

Artem Bityutskiy

2012-06-29 11:25 UTC #8 lore raw

PATCH 1/2 mtd mxc_nand: use 32bit copy functions

Quoted 3 lines · Uwe Kleine-K?nig

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.

Signature · 9 lines

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/da941a49/attachment.sig>

Uwe Kleine-König

2012-06-29 11:28 UTC #9 lore raw

PATCH 1/2 mtd mxc_nand: use 32bit copy functions

Hi Artem,

Quoted 24 lines · Artem Bityutskiy

Quoted 3 lines · Uwe Kleine-K?nig

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:

(quoted diff) +14 -0

+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]

Quoted 1 line

The "volitile" should be also removed.

Did you see that Sascha posted a v2?

Best regards
Uwe

Signature · 2 lines

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