diff mbox

[RFC,23/47] mtd: nand: stm_nand_bch: read and write page (BCH)

Message ID 1395735604-26706-24-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones March 25, 2014, 8:19 a.m. UTC
Use DMA to read and/or write a single page of data.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/nand/stm_nand_bch.c | 119 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

Comments

Pekon Gupta March 26, 2014, 10:17 a.m. UTC | #1
Hi Lee,

>From: Lee Jones [mailto:lee.jones@linaro.org]
>
>Use DMA to read and/or write a single page of data.
>
>Signed-off-by: Lee Jones <lee.jones@linaro.org>
>---
> drivers/mtd/nand/stm_nand_bch.c | 119 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 119 insertions(+)
>
>diff --git a/drivers/mtd/nand/stm_nand_bch.c b/drivers/mtd/nand/stm_nand_bch.c
>index 7874d85..6323590 100644
>--- a/drivers/mtd/nand/stm_nand_bch.c
>+++ b/drivers/mtd/nand/stm_nand_bch.c
>@@ -21,6 +21,7 @@
> #include <linux/interrupt.h>
> #include <linux/device.h>
> #include <linux/platform_device.h>
>+#include <linux/dma-mapping.h>
> #include <linux/completion.h>
> #include <linux/mtd/nand.h>
> #include <linux/mtd/stm_nand.h>
>@@ -345,6 +346,124 @@ static int check_erased_page(uint8_t *data, uint32_t page_size, int max_zeros)
> 	return zeros;
> }
>
>+/* Returns the number of ECC errors, or '-1' for uncorrectable error */
>+static int bch_read_page(struct nandi_controller *nandi,
>+			 loff_t offs,
>+			 uint8_t *buf)
>+{
>+	struct bch_prog *prog = &bch_prog_read_page;
>+	uint32_t page_size = nandi->info.mtd.writesize;
>+	unsigned long list_phys;
>+	unsigned long buf_phys;
>+	uint32_t ecc_err;
>+	int ret = 0;
>+
>+	dev_dbg(nandi->dev, "%s: offs = 0x%012llx\n", __func__, offs);
>+
>+	BUG_ON((unsigned long)buf & (NANDI_BCH_DMA_ALIGNMENT - 1));
>+	BUG_ON(offs & (NANDI_BCH_DMA_ALIGNMENT - 1));
>+
>+	emiss_nandi_select(STM_NANDI_BCH);
>+
>+	nandi_enable_interrupts(nandi, NANDBCH_INT_SEQNODESOVER);
>+	reinit_completion(&nandi->seq_completed);
>+
>+	/* Reset ECC stats */
>+	writel(CFG_RESET_ECC_ALL | CFG_ENABLE_AFM,
>+	       nandi->base + NANDBCH_CONTROLLER_CFG);
>+	writel(CFG_ENABLE_AFM, nandi->base + NANDBCH_CONTROLLER_CFG);
>+
>+	prog->addr = (uint32_t)((offs >> (nandi->page_shift - 8)) & 0xffffff00);
>+
>+	buf_phys = dma_map_single(NULL, buf, page_size, DMA_FROM_DEVICE);
>+
>+	memset(nandi->buf_list, 0x00, NANDI_BCH_BUF_LIST_SIZE);
>+	nandi->buf_list[0] = buf_phys | (nandi->sectors_per_page - 1);
>+
>+	list_phys = dma_map_single(NULL, nandi->buf_list,
>+				   NANDI_BCH_BUF_LIST_SIZE, DMA_TO_DEVICE);
>+
>+	writel(list_phys, nandi->base + NANDBCH_BUFFER_LIST_PTR);
>+
>+	bch_load_prog_cpu(nandi, prog);
>+
>+	bch_wait_seq(nandi);
>+
>+	nandi_disable_interrupts(nandi, NANDBCH_INT_SEQNODESOVER);
>+
>+	dma_unmap_single(NULL, list_phys, NANDI_BCH_BUF_LIST_SIZE,
>+			 DMA_TO_DEVICE);
>+	dma_unmap_single(NULL, buf_phys, page_size, DMA_FROM_DEVICE);
>+
>+	/* Use the maximum per-sector ECC count! */

Firstly, this ecc checking and correction should not be part of bch_read_page().
This should be part of chip->ecc.correct().
But, I don't see your driver using nand_chip->ecc interfaces.
Why do you want to break away from generic driver flow ? any controller limitation ?

I think much of the code in below patch can be reused from nand_base.c
	[RFC 43/47] mtd: nand: stm_nand_bch: read and write functions (BCH)


>+	ecc_err = readl(nandi->base + NANDBCH_ECC_SCORE_REG_A) & 0xff;
>+	if (ecc_err == 0xff) {
>+		/*
>+		 * Downgrade uncorrectable ECC error for an erased page,
>+		 * tolerating 'sectors_per_page' bits at zero.
>+		 */
>+		ret = check_erased_page(buf, page_size,
>+					nandi->sectors_per_page);

This is also not correct. Here 'max_zeros' should be ecc.strength


>+		if (ret >= 0)
>+			dev_dbg(nandi->dev,
>+				"%s: erased page detected: \n"
>+				"  downgrading uncorrectable ECC error.\n",
>+				__func__);
>+	} else {
>+		ret = (int)ecc_err;
>+	}
>+
>+	return ret;
>+}
>+

with regards, pekon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Lee Jones April 30, 2014, 11:19 a.m. UTC | #2
> >From: Lee Jones [mailto:lee.jones@linaro.org]
> >
> >Use DMA to read and/or write a single page of data.
> >
> >Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >---
> > drivers/mtd/nand/stm_nand_bch.c | 119 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 119 insertions(+)
> >
> >diff --git a/drivers/mtd/nand/stm_nand_bch.c b/drivers/mtd/nand/stm_nand_bch.c
> >index 7874d85..6323590 100644
> >--- a/drivers/mtd/nand/stm_nand_bch.c
> >+++ b/drivers/mtd/nand/stm_nand_bch.c
> >@@ -21,6 +21,7 @@

[...]

> >+/* Returns the number of ECC errors, or '-1' for uncorrectable error */
> >+static int bch_read_page(struct nandi_controller *nandi,
> >+			 loff_t offs,
> >+			 uint8_t *buf)
> >+{

[...]

> >+	/* Use the maximum per-sector ECC count! */
> 
> Firstly, this ecc checking and correction should not be part of bch_read_page().
> This should be part of chip->ecc.correct().

I'm not sure that's true.  The functionality below isn't correcting
any data.  I believe it's preventing a false-positive error return.
All calculate() and correct() behaviour is dealt with internally by
h/w.  There should be no intervention from s/w, hence the lack of
call-back functions of the same name.

> But, I don't see your driver using nand_chip->ecc interfaces.
> Why do you want to break away from generic driver flow ? any controller limitation ?

When this driver was written, a great deal of the framework was not
present.  I believe the author also wanted to prevent any changes in
the framework from adversely affecting the leaf driver.  It was
considered that if there is no intention to upstream, it would be much
better to hack around the framework and have full control of the
driver.

Things are different now, as upstreaming is considered necessary.
It's my job to make this driver acceptable so it can be Mainlined.
This includes a 'port' to ensure the driver is using the correct
interfaces provided by the framework.

> I think much of the code in below patch can be reused from nand_base.c
> 	[RFC 43/47] mtd: nand: stm_nand_bch: read and write functions (BCH)

I agree and have taken care of this now.

> >+	ecc_err = readl(nandi->base + NANDBCH_ECC_SCORE_REG_A) & 0xff;
> >+	if (ecc_err == 0xff) {
> >+		/*
> >+		 * Downgrade uncorrectable ECC error for an erased page,
> >+		 * tolerating 'sectors_per_page' bits at zero.
> >+		 */
> >+		ret = check_erased_page(buf, page_size,
> >+					nandi->sectors_per_page);
> 
> This is also not correct. Here 'max_zeros' should be ecc.strength

I need to check this with the original author, but I'm inclined to
agree with you.  I'll make the changes before re-submitting.

> >+		if (ret >= 0)
> >+			dev_dbg(nandi->dev,
> >+				"%s: erased page detected: \n"
> >+				"  downgrading uncorrectable ECC error.\n",
> >+				__func__);
> >+	} else {
> >+		ret = (int)ecc_err;
> >+	}
> >+
> >+	return ret;
> >+}
> >+
> 
> with regards, pekon
diff mbox

Patch

diff --git a/drivers/mtd/nand/stm_nand_bch.c b/drivers/mtd/nand/stm_nand_bch.c
index 7874d85..6323590 100644
--- a/drivers/mtd/nand/stm_nand_bch.c
+++ b/drivers/mtd/nand/stm_nand_bch.c
@@ -21,6 +21,7 @@ 
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
 #include <linux/completion.h>
 #include <linux/mtd/nand.h>
 #include <linux/mtd/stm_nand.h>
@@ -345,6 +346,124 @@  static int check_erased_page(uint8_t *data, uint32_t page_size, int max_zeros)
 	return zeros;
 }
 
+/* Returns the number of ECC errors, or '-1' for uncorrectable error */
+static int bch_read_page(struct nandi_controller *nandi,
+			 loff_t offs,
+			 uint8_t *buf)
+{
+	struct bch_prog *prog = &bch_prog_read_page;
+	uint32_t page_size = nandi->info.mtd.writesize;
+	unsigned long list_phys;
+	unsigned long buf_phys;
+	uint32_t ecc_err;
+	int ret = 0;
+
+	dev_dbg(nandi->dev, "%s: offs = 0x%012llx\n", __func__, offs);
+
+	BUG_ON((unsigned long)buf & (NANDI_BCH_DMA_ALIGNMENT - 1));
+	BUG_ON(offs & (NANDI_BCH_DMA_ALIGNMENT - 1));
+
+	emiss_nandi_select(STM_NANDI_BCH);
+
+	nandi_enable_interrupts(nandi, NANDBCH_INT_SEQNODESOVER);
+	reinit_completion(&nandi->seq_completed);
+
+	/* Reset ECC stats */
+	writel(CFG_RESET_ECC_ALL | CFG_ENABLE_AFM,
+	       nandi->base + NANDBCH_CONTROLLER_CFG);
+	writel(CFG_ENABLE_AFM, nandi->base + NANDBCH_CONTROLLER_CFG);
+
+	prog->addr = (uint32_t)((offs >> (nandi->page_shift - 8)) & 0xffffff00);
+
+	buf_phys = dma_map_single(NULL, buf, page_size, DMA_FROM_DEVICE);
+
+	memset(nandi->buf_list, 0x00, NANDI_BCH_BUF_LIST_SIZE);
+	nandi->buf_list[0] = buf_phys | (nandi->sectors_per_page - 1);
+
+	list_phys = dma_map_single(NULL, nandi->buf_list,
+				   NANDI_BCH_BUF_LIST_SIZE, DMA_TO_DEVICE);
+
+	writel(list_phys, nandi->base + NANDBCH_BUFFER_LIST_PTR);
+
+	bch_load_prog_cpu(nandi, prog);
+
+	bch_wait_seq(nandi);
+
+	nandi_disable_interrupts(nandi, NANDBCH_INT_SEQNODESOVER);
+
+	dma_unmap_single(NULL, list_phys, NANDI_BCH_BUF_LIST_SIZE,
+			 DMA_TO_DEVICE);
+	dma_unmap_single(NULL, buf_phys, page_size, DMA_FROM_DEVICE);
+
+	/* Use the maximum per-sector ECC count! */
+	ecc_err = readl(nandi->base + NANDBCH_ECC_SCORE_REG_A) & 0xff;
+	if (ecc_err == 0xff) {
+		/*
+		 * Downgrade uncorrectable ECC error for an erased page,
+		 * tolerating 'sectors_per_page' bits at zero.
+		 */
+		ret = check_erased_page(buf, page_size,
+					nandi->sectors_per_page);
+		if (ret >= 0)
+			dev_dbg(nandi->dev,
+				"%s: erased page detected: \n"
+				"  downgrading uncorrectable ECC error.\n",
+				__func__);
+	} else {
+		ret = (int)ecc_err;
+	}
+
+	return ret;
+}
+
+/* Returns the status of the NAND device following the write operation */
+static uint8_t bch_write_page(struct nandi_controller *nandi,
+			      loff_t offs, const uint8_t *buf)
+{
+	struct bch_prog *prog = &bch_prog_write_page;
+	uint32_t page_size = nandi->info.mtd.writesize;
+	uint8_t *p = (uint8_t *)buf;
+	unsigned long list_phys;
+	unsigned long buf_phys;
+	uint8_t status;
+
+	dev_dbg(nandi->dev, "%s: offs = 0x%012llx\n", __func__, offs);
+
+	BUG_ON((unsigned int)buf & (NANDI_BCH_DMA_ALIGNMENT - 1));
+	BUG_ON(offs & (page_size - 1));
+
+	emiss_nandi_select(STM_NANDI_BCH);
+
+	nandi_enable_interrupts(nandi, NANDBCH_INT_SEQNODESOVER);
+	reinit_completion(&nandi->seq_completed);
+
+	prog->addr = (uint32_t)((offs >> (nandi->page_shift - 8)) & 0xffffff00);
+
+	buf_phys = dma_map_single(NULL, p, page_size, DMA_TO_DEVICE);
+	memset(nandi->buf_list, 0x00, NANDI_BCH_BUF_LIST_SIZE);
+	nandi->buf_list[0] = buf_phys | (nandi->sectors_per_page - 1);
+
+	list_phys = dma_map_single(NULL, nandi->buf_list,
+				   NANDI_BCH_BUF_LIST_SIZE, DMA_TO_DEVICE);
+
+	writel(list_phys, nandi->base + NANDBCH_BUFFER_LIST_PTR);
+
+	bch_load_prog_cpu(nandi, prog);
+
+	bch_wait_seq(nandi);
+
+	nandi_disable_interrupts(nandi, NANDBCH_INT_SEQNODESOVER);
+
+	dma_unmap_single(NULL, list_phys, NANDI_BCH_BUF_LIST_SIZE,
+			 DMA_TO_DEVICE);
+	dma_unmap_single(NULL, buf_phys, page_size, DMA_FROM_DEVICE);
+
+	status = (uint8_t)(readl(nandi->base +
+				 NANDBCH_CHECK_STATUS_REG_A) & 0xff);
+
+	return status;
+}
+
 /*
  * Initialisation
  */