2012-02-02 12:04:16

by Artem Bityutskiy

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

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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
Acked-by: Nicolas Ferre <[email protected]>
Signed-off-by: Artem Bityutskiy <[email protected]>
Cc: [email protected] [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


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2012-02-03 03:35:42

by Venu Byravarasu

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

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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2012-02-03 05:30:43

by Artem Bityutskiy

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

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


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part