| Message ID | 1337955762-19157-1-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
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);
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
On 05/25/2012 07:22 AM, Sascha Hauer wrote: > Since this commit: > > commit 6a918bade9dab40aaef80559bd1169c69e8d69cb > Author: Mike Dunn <mikedunn@newsguy.com> > 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. Oops, sorry Sascha. This was compile-tested only, but I should have ensured strength was set before call to nand_scan_tail(). Thanks, Mike
On Fri, 2012-05-25 at 16:22 +0200, Sascha Hauer wrote: > Since this commit: > > commit 6a918bade9dab40aaef80559bd1169c69e8d69cb > Author: Mike Dunn <mikedunn@newsguy.com> > 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. Pushed this one to l2-mtd.git as well, thanks!
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
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.
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
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.
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);
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(-)