| Message ID | 1338279369-25915-1-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | 096bcc231fd263bc8df215f0d616b08e3696c6db |
| Headers | show |
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 > >
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
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...
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
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!
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
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(-)
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);
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(-)