* [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices @ 2012-01-18 9:16 Nikolaus Voss 2012-01-27 13:29 ` Artem Bityutskiy 0 siblings, 1 reply; 8+ messages in thread From: Nikolaus Voss @ 2012-01-18 9:16 UTC (permalink / raw) To: Artem Bityutskiy, Nicolas Ferre; +Cc: linux-mtd, linux-kernel commit fb5427508abbd635e877fabdf55795488119c2d6 optimizes PIO NAND accesses by using IO memcpy instead of IO read/write repeated functions. This breaks access to 16 bit NAND devices as memcpy_fromio()/toio() _always_ use byte accesses (see arch/arm/kernel/io.c), so with 16 bit NAND, one byte gets lost per NAND access cycle and NAND address count is wrong. Using memcpy() instead of the IO memcpy functions fixes this, but depends on correct word alignment of the buffer and length has to be a multiple of four, otherwise it might issue byte accesses and possibly break 16 bit NAND access (cf arch/arm/lib/copy_template.S). Memcpy variants seem to be the wrong approach here, since the NAND controller doesn't make the NAND appear as truely randomly accessible memory (as opposed to the DRAM controller which does exactly that). So, my proposal is to use 32 bit IO read/write (and let SMC map it to 8 bit or 16 bit NAND accesses) and account for length % 4 > 0 with two additional IO read/writes. Signed-off-by: Nikolaus Voss <n.voss@weinmann.de> --- drivers/mtd/nand/atmel_nand.c | 19 +++++++++++++++++-- 1 files changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c index a582a0f..7396d4a 100644 --- a/drivers/mtd/nand/atmel_nand.c +++ b/drivers/mtd/nand/atmel_nand.c @@ -265,7 +265,12 @@ static void atmel_read_buf(struct mtd_info *mtd, u8 *buf, int len) return; /* if no DMA operation possible, use PIO */ - memcpy_fromio(buf, chip->IO_ADDR_R, len); + readsl(chip->IO_ADDR_R, buf, len >> 2); + if (len & 2) + /* read into buf[len - 2] or buf[len - 3] */ + *((u16 *)&buf[len & ~3]) = chip->read_word(mtd); + if (len & 1) + buf[len - 1] = chip->read_byte(mtd); } static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len) @@ -278,7 +283,17 @@ static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len) return; /* if no DMA operation possible, use PIO */ - memcpy_toio(chip->IO_ADDR_W, buf, len); + writesl(chip->IO_ADDR_W, buf, len >> 2); + if (len & 2) + /* write from buf[len - 2] or buf[len - 3] */ + writew(*((u16 *)&buf[len & ~3]), chip->IO_ADDR_R); + if (len & 1) { + /* a single byte cannot be written to 16 bit NAND */ + if (chip->options & NAND_BUSWIDTH_16) + BUG(); + else + writeb(buf[len - 1], chip->IO_ADDR_R); + } } /* -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices 2012-01-18 9:16 [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices Nikolaus Voss @ 2012-01-27 13:29 ` Artem Bityutskiy 2012-01-30 7:57 ` Voss, Nikolaus 0 siblings, 1 reply; 8+ messages in thread From: Artem Bityutskiy @ 2012-01-27 13:29 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Nicolas Ferre, linux-mtd, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1335 bytes --] On Wed, 2012-01-18 at 10:16 +0100, Nikolaus Voss wrote: > commit fb5427508abbd635e877fabdf55795488119c2d6 optimizes PIO > NAND accesses by using IO memcpy instead of IO read/write > repeated functions. > > This breaks access to 16 bit NAND devices as memcpy_fromio()/toio() > _always_ use byte accesses (see arch/arm/kernel/io.c), so with > 16 bit NAND, one byte gets lost per NAND access cycle and NAND > address count is wrong. > > Using memcpy() instead of the IO memcpy functions fixes this, but > depends on correct word alignment of the buffer and length has to > be a multiple of four, otherwise it might issue byte accesses and > possibly break 16 bit NAND access (cf arch/arm/lib/copy_template.S). > > Memcpy variants seem to be the wrong approach here, since the > NAND controller doesn't make the NAND appear as truely randomly > accessible memory (as opposed to the DRAM controller which does > exactly that). > > So, my proposal is to use 32 bit IO read/write (and let SMC > map it to 8 bit or 16 bit NAND accesses) and account for > length % 4 > 0 with two additional IO read/writes. > > Signed-off-by: Nikolaus Voss <n.voss@weinmann.de> Why not to revert fb5427508abbd635e877fabdf55795488119c2d6 instead, it is in my opinion a bit more readable? -- Best Regards, Artem Bityutskiy [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices 2012-01-27 13:29 ` Artem Bityutskiy @ 2012-01-30 7:57 ` Voss, Nikolaus 2012-01-30 8:57 ` Nicolas Ferre 0 siblings, 1 reply; 8+ messages in thread From: Voss, Nikolaus @ 2012-01-30 7:57 UTC (permalink / raw) To: 'dedekind1@gmail.com' Cc: 'Nicolas Ferre', 'linux-mtd@lists.infradead.org', 'linux-kernel@vger.kernel.org' [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1651 bytes --] Artem Bityutskiy wrote on 2012-01-27: > On Wed, 2012-01-18 at 10:16 +0100, Nikolaus Voss wrote: >> commit fb5427508abbd635e877fabdf55795488119c2d6 optimizes PIO >> NAND accesses by using IO memcpy instead of IO read/write >> repeated functions. >> >> This breaks access to 16 bit NAND devices as memcpy_fromio()/toio() >> _always_ use byte accesses (see arch/arm/kernel/io.c), so with >> 16 bit NAND, one byte gets lost per NAND access cycle and NAND >> address count is wrong. >> >> Using memcpy() instead of the IO memcpy functions fixes this, but >> depends on correct word alignment of the buffer and length has to >> be a multiple of four, otherwise it might issue byte accesses and >> possibly break 16 bit NAND access (cf arch/arm/lib/copy_template.S). >> >> Memcpy variants seem to be the wrong approach here, since the >> NAND controller doesn't make the NAND appear as truely randomly >> accessible memory (as opposed to the DRAM controller which does >> exactly that). >> >> So, my proposal is to use 32 bit IO read/write (and let SMC >> map it to 8 bit or 16 bit NAND accesses) and account for >> length % 4 > 0 with two additional IO read/writes. >> >> Signed-off-by: Nikolaus Voss <n.voss@weinmann.de> > > Why not to revert fb5427508abbd635e877fabdf55795488119c2d6 instead, it > is in my opinion a bit more readable? No objections. I've tried to save the idea of fb5427508abbd635e877fabdf55795488119c2d6 but that length % 4 > 0 stuff uglifies it a little bit... Niko ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices 2012-01-30 7:57 ` Voss, Nikolaus @ 2012-01-30 8:57 ` Nicolas Ferre 2012-02-01 10:43 ` Eric Bénard 2012-02-02 12:06 ` Artem Bityutskiy 0 siblings, 2 replies; 8+ messages in thread From: Nicolas Ferre @ 2012-01-30 8:57 UTC (permalink / raw) To: Voss, Nikolaus, 'dedekind1@gmail.com' Cc: 'linux-mtd@lists.infradead.org', 'linux-kernel@vger.kernel.org' On 01/30/2012 08:57 AM, Voss, Nikolaus : > Artem Bityutskiy wrote on 2012-01-27: >> On Wed, 2012-01-18 at 10:16 +0100, Nikolaus Voss wrote: >>> commit fb5427508abbd635e877fabdf55795488119c2d6 optimizes PIO >>> NAND accesses by using IO memcpy instead of IO read/write >>> repeated functions. >>> >>> This breaks access to 16 bit NAND devices as memcpy_fromio()/toio() >>> _always_ use byte accesses (see arch/arm/kernel/io.c), so with >>> 16 bit NAND, one byte gets lost per NAND access cycle and NAND >>> address count is wrong. >>> >>> Using memcpy() instead of the IO memcpy functions fixes this, but >>> depends on correct word alignment of the buffer and length has to >>> be a multiple of four, otherwise it might issue byte accesses and >>> possibly break 16 bit NAND access (cf arch/arm/lib/copy_template.S). >>> >>> Memcpy variants seem to be the wrong approach here, since the >>> NAND controller doesn't make the NAND appear as truely randomly >>> accessible memory (as opposed to the DRAM controller which does >>> exactly that). >>> >>> So, my proposal is to use 32 bit IO read/write (and let SMC >>> map it to 8 bit or 16 bit NAND accesses) and account for >>> length % 4 > 0 with two additional IO read/writes. >>> >>> Signed-off-by: Nikolaus Voss <n.voss@weinmann.de> >> >> Why not to revert fb5427508abbd635e877fabdf55795488119c2d6 instead, it >> is in my opinion a bit more readable? > > No objections. I've tried to save the idea of > fb5427508abbd635e877fabdf55795488119c2d6 but that length % 4 > 0 stuff > uglifies it a little bit... After double checking with designers, I must admit that I misunderstood the way of optimizing accesses to SMC. 16 bit nand is not so common those days... So yes, Nikolaus your correction makes sense. What I would have liked is to optimize the possibility to trigger incremental bursts from the processor to the SMC on internal bus. I suspect that this will need further investigation (moreover, note that if we use assembly code, we should also think about AVR32). In conclusion, maybe simply reverting the initial commit will allow us to rework this part of code from simpler basis. Artem, do you want me to prepare a patch for reverting initial commit or you just need my "Acked-by" (feel free to add though)? Best regards, -- Nicolas Ferre ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices 2012-01-30 8:57 ` Nicolas Ferre @ 2012-02-01 10:43 ` Eric Bénard 2012-02-02 12:06 ` Artem Bityutskiy 1 sibling, 0 replies; 8+ messages in thread From: Eric Bénard @ 2012-02-01 10:43 UTC (permalink / raw) To: Nicolas Ferre Cc: Voss, Nikolaus, 'dedekind1@gmail.com', 'linux-mtd@lists.infradead.org', 'linux-kernel@vger.kernel.org' Good morning, Le Mon, 30 Jan 2012 09:57:34 +0100, Nicolas Ferre <nicolas.ferre@atmel.com> a écrit : > On 01/30/2012 08:57 AM, Voss, Nikolaus : > > Artem Bityutskiy wrote on 2012-01-27: > >> On Wed, 2012-01-18 at 10:16 +0100, Nikolaus Voss wrote: > >>> commit fb5427508abbd635e877fabdf55795488119c2d6 optimizes PIO > >>> NAND accesses by using IO memcpy instead of IO read/write > >>> repeated functions. > >>> > >>> This breaks access to 16 bit NAND devices as memcpy_fromio()/toio() > >>> _always_ use byte accesses (see arch/arm/kernel/io.c), so with > >>> 16 bit NAND, one byte gets lost per NAND access cycle and NAND > >>> address count is wrong. > >>> > >>> Using memcpy() instead of the IO memcpy functions fixes this, but > >>> depends on correct word alignment of the buffer and length has to > >>> be a multiple of four, otherwise it might issue byte accesses and > >>> possibly break 16 bit NAND access (cf arch/arm/lib/copy_template.S). > >>> > >>> Memcpy variants seem to be the wrong approach here, since the > >>> NAND controller doesn't make the NAND appear as truely randomly > >>> accessible memory (as opposed to the DRAM controller which does > >>> exactly that). > >>> > >>> So, my proposal is to use 32 bit IO read/write (and let SMC > >>> map it to 8 bit or 16 bit NAND accesses) and account for > >>> length % 4 > 0 with two additional IO read/writes. > >>> > >>> Signed-off-by: Nikolaus Voss <n.voss@weinmann.de> > >> > >> Why not to revert fb5427508abbd635e877fabdf55795488119c2d6 instead, it > >> is in my opinion a bit more readable? > > > > No objections. I've tried to save the idea of > > fb5427508abbd635e877fabdf55795488119c2d6 but that length % 4 > 0 stuff > > uglifies it a little bit... > > After double checking with designers, I must admit that I misunderstood > the way of optimizing accesses to SMC. 16 bit nand is not so common > those days... > > So yes, Nikolaus your correction makes sense. > > What I would have liked is to optimize the possibility to trigger > incremental bursts from the processor to the SMC on internal bus. I > suspect that this will need further investigation (moreover, note that > if we use assembly code, we should also think about AVR32). > > In conclusion, maybe simply reverting the initial commit will allow us > to rework this part of code from simpler basis. > > Artem, do you want me to prepare a patch for reverting initial commit or > you just need my "Acked-by" (feel free to add though)? > I confirm this patch breaks 16 bits NAND flash (tested on a SAM9G45 with linux-3.2.2) and that reverting it solves the problem. Best regards Eric http://eukrea.com/en/news/104-2012 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices 2012-01-30 8:57 ` Nicolas Ferre 2012-02-01 10:43 ` Eric Bénard @ 2012-02-02 12:06 ` Artem Bityutskiy 2012-02-03 3:35 ` Venu Byravarasu 1 sibling, 1 reply; 8+ messages in thread From: Artem Bityutskiy @ 2012-02-02 12:06 UTC (permalink / raw) To: Nicolas Ferre, Voss, Nikolaus, Eric Bénard Cc: 'linux-mtd@lists.infradead.org', 'linux-kernel@vger.kernel.org' [-- Attachment #1: Type: text/plain, Size: 3816 bytes --] On Mon, 2012-01-30 at 09:57 +0100, Nicolas Ferre wrote: > Artem, do you want me to prepare a patch for reverting initial commit or > you just need my "Acked-by" (feel free to add though)? OK, thanks, just pushed this patch to l2-mtd.git and add Cc to -stable, please, validate. I'll ask David to merge it to 3.3, but no guarantees - you should ping him directly if you want this to happen. URL: http://git.infradead.org/users/dedekind/l2-mtd.git/commit/21c7726c98628016c868d803a2a8e6f2d5702519 From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> Date: Thu, 2 Feb 2012 13:54:25 +0200 Subject: [PATCH] Revert "mtd: atmel_nand: optimize read/write buffer functions" This reverts commit fb5427508abbd635e877fabdf55795488119c2d6. The reason is that it breaks 16 bits NAND flash as it was reported by Nikolaus Voss and confirmed by Eric Bénard. Nicolas Ferre <nicolas.ferre@atmel.com> alco confirmed: "After double checking with designers, I must admit that I misunderstood the way of optimizing accesses to SMC. 16 bit nand is not so common those days..." Reported-by: Nikolaus Voss <n.voss@weinmann.de> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> Cc: stable@kernel.org [3.1+] --- drivers/mtd/nand/atmel_nand.c | 45 +++++++++++++++++++++++++++++++++++++--- 1 files changed, 41 insertions(+), 4 deletions(-) diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c index 4dd056e..35b4fb5 100644 --- a/drivers/mtd/nand/atmel_nand.c +++ b/drivers/mtd/nand/atmel_nand.c @@ -161,6 +161,37 @@ static int atmel_nand_device_ready(struct mtd_info *mtd) !!host->board->rdy_pin_active_low; } +/* + * Minimal-overhead PIO for data access. + */ +static void atmel_read_buf8(struct mtd_info *mtd, u8 *buf, int len) +{ + struct nand_chip *nand_chip = mtd->priv; + + __raw_readsb(nand_chip->IO_ADDR_R, buf, len); +} + +static void atmel_read_buf16(struct mtd_info *mtd, u8 *buf, int len) +{ + struct nand_chip *nand_chip = mtd->priv; + + __raw_readsw(nand_chip->IO_ADDR_R, buf, len / 2); +} + +static void atmel_write_buf8(struct mtd_info *mtd, const u8 *buf, int len) +{ + struct nand_chip *nand_chip = mtd->priv; + + __raw_writesb(nand_chip->IO_ADDR_W, buf, len); +} + +static void atmel_write_buf16(struct mtd_info *mtd, const u8 *buf, int len) +{ + struct nand_chip *nand_chip = mtd->priv; + + __raw_writesw(nand_chip->IO_ADDR_W, buf, len / 2); +} + static void dma_complete_func(void *completion) { complete(completion); @@ -235,27 +266,33 @@ err_buf: static void atmel_read_buf(struct mtd_info *mtd, u8 *buf, int len) { struct nand_chip *chip = mtd->priv; + struct atmel_nand_host *host = chip->priv; if (use_dma && len > mtd->oobsize) /* only use DMA for bigger than oob size: better performances */ if (atmel_nand_dma_op(mtd, buf, len, 1) == 0) return; - /* if no DMA operation possible, use PIO */ - memcpy_fromio(buf, chip->IO_ADDR_R, len); + if (host->board->bus_width_16) + atmel_read_buf16(mtd, buf, len); + else + atmel_read_buf8(mtd, buf, len); } static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len) { struct nand_chip *chip = mtd->priv; + struct atmel_nand_host *host = chip->priv; if (use_dma && len > mtd->oobsize) /* only use DMA for bigger than oob size: better performances */ if (atmel_nand_dma_op(mtd, (void *)buf, len, 0) == 0) return; - /* if no DMA operation possible, use PIO */ - memcpy_toio(chip->IO_ADDR_W, buf, len); + if (host->board->bus_width_16) + atmel_write_buf16(mtd, buf, len); + else + atmel_write_buf8(mtd, buf, len); } /* -- 1.7.9 -- Best Regards, Artem Bityutskiy [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices 2012-02-02 12:06 ` Artem Bityutskiy @ 2012-02-03 3:35 ` Venu Byravarasu 2012-02-03 5:32 ` Artem Bityutskiy 0 siblings, 1 reply; 8+ messages in thread From: Venu Byravarasu @ 2012-02-03 3:35 UTC (permalink / raw) To: dedekind1, Nicolas Ferre, Voss, Nikolaus, Eric Bénard Cc: 'linux-mtd@lists.infradead.org', 'linux-kernel@vger.kernel.org' [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 691 bytes --] I see a problem with atmel_read_buf16 & atmel_write_buf16 functions. As they are calling __raw_readsw & __raw_writesw respectively, which would cause panic in cases of 16 bit unaligned buffers. However, the buf passed to these functions is of type u8* from atmel_read_buf & atmel_write_buf functions. [Venu] > - /* if no DMA operation possible, use PIO */ > - memcpy_fromio(buf, chip->IO_ADDR_R, len); > + if (host->board->bus_width_16) > + atmel_read_buf16(mtd, buf, len); > + else > + atmel_read_buf8(mtd, buf, len); > } ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices 2012-02-03 3:35 ` Venu Byravarasu @ 2012-02-03 5:32 ` Artem Bityutskiy 0 siblings, 0 replies; 8+ messages in thread From: Artem Bityutskiy @ 2012-02-03 5:32 UTC (permalink / raw) To: Venu Byravarasu Cc: Nicolas Ferre, Voss, Nikolaus, Eric Bénard, 'linux-mtd@lists.infradead.org', 'linux-kernel@vger.kernel.org' [-- Attachment #1: Type: text/plain, Size: 773 bytes --] On Fri, 2012-02-03 at 09:05 +0530, Venu Byravarasu wrote: > I see a problem with atmel_read_buf16 & atmel_write_buf16 functions. > As they are calling __raw_readsw & __raw_writesw respectively, which > would cause panic in cases of 16 bit unaligned buffers. > However, the buf passed to these functions is of type u8* from > atmel_read_buf & atmel_write_buf functions. I think this is a valid concern. In theory, the NAND infrustructure must guarantee that the buffer is always of even length, and most probably it does, but we could add a WARN_ON(len & 1) statements in 'atmel_read_buf()' and 'atmel_write_buf()'. And probably the type could indeed be changed. But these concerns should be addressed separately. -- Best Regards, Artem Bityutskiy [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-02-03 5:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-18 9:16 [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices Nikolaus Voss 2012-01-27 13:29 ` Artem Bityutskiy 2012-01-30 7:57 ` Voss, Nikolaus 2012-01-30 8:57 ` Nicolas Ferre 2012-02-01 10:43 ` Eric Bénard 2012-02-02 12:06 ` Artem Bityutskiy 2012-02-03 3:35 ` Venu Byravarasu 2012-02-03 5:32 ` Artem Bityutskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).