Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices

Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices

[Artem Bityutskiy]

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

RE: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices

[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...

Niko

Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices

[Nicolas Ferre]

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

Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices

[Eric Bénard]

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

Re: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices

[Artem Bityutskiy]

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

RE: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices

[Venu Byravarasu]

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

RE: [PATCH] mtd: atmel_nand: fix access to 16 bit NAND devices

[Artem Bityutskiy]

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