GPMI nand driver cleanup

GPMI nand driver cleanup

[Sascha Hauer]

This series contains several cleanup patches for the GPMI nand driver.
The code is unnecessarily complicated in several places. For example
the private driver data struct contains variables which should better
be passed around between functions to make the lifetime of these
variables clear. Also there are functions which take up to eight
arguements and are so trivial that they are shorter and easier to
read if open coded in the caller.

The series also contains the bugfix in 1/10 which I previously sent
separately.

The driver has more room for cleanups. The splitting up into gpmi-lib
and gpmi-nand seems rather unnecessary. The gpmi-lib functions call
back into gpmi-nand which contradicts the principle of a lib. Also
the functions in gpmi-lib all have a proper gpmi_ prefix, but the
functions exported from gpmi-nand.c do not have a prefix at all. In
several cases gpmi-nand.c only contains a wrapper around functions
in gpmi-lib.c. I tend to merge gpmi-nand.c and gpmi-lib.c into a
single file (which would be around 3.7k lines then). I think this
would make the driver a lot more readable, but of course also produces
some churn. Before doing this I want to ask for other opinions to avoid
putting the work to /dev/null later.

Sascha

----------------------------------------------------------------
Sascha Hauer (10):
      mtd: nand: gpmi: Fix failure when a erased page has a bitflip at BBM
      mtd: nand: gpmi: Utilize hardware to detect bitflips in erased blocks
      mtd: nand: gpmi: drop dma_ops_type
      mtd: nand: gpmi: pass buffer and len around
      mtd: nand: gpmi: put only once used functions inline
      mtd: nand: gpmi: remove direct_dma_map_ok from driver data struct
      mtd: nand: gpmi: return valid value from bch_set_geometry()
      mtd: nand: gpmi: remove unnecessary variables
      mtd: nand: gpmi: rework gpmi_ecc_write_page
      mtd: nand: gpmi: return error code from gpmi_ecc_write_page

 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |  55 +++--
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 383 ++++++++-------------------------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  24 +--
 3 files changed, 128 insertions(+), 334 deletions(-)

[PATCH 01/10] mtd: nand: gpmi: Fix failure when a erased page has a bitflip at BBM

[Sascha Hauer]

When erased subpages are read then the BCH decoder returns STATUS_ERASED
if they are all empty, or STATUS_UNCORRECTABLE if there are bitflips.
When there are bitflips, we have to set these bytes again to show the
upper layers a completely erased page. When a bitflip happens in the
exact byte where the bad block marker is, then this byte is swapped
with another byte in block_mark_swapping(). The correction code then
detects a bitflip in another subpage and no longer corrects the bitflip
where it really happens.

Correct this behaviour by calling block_mark_swapping() after the
bitflips have been corrected.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 50f8d4a1b983..d4d824ef64e9 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1067,9 +1067,6 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 		return ret;
 	}
 
-	/* handle the block mark swapping */
-	block_mark_swapping(this, payload_virt, auxiliary_virt);
-
 	/* Loop over status bytes, accumulating ECC status. */
 	status = auxiliary_virt + nfc_geo->auxiliary_status_offset;
 
@@ -1158,6 +1155,9 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 		max_bitflips = max_t(unsigned int, max_bitflips, *status);
 	}
 
+	/* handle the block mark swapping */
+	block_mark_swapping(this, buf, auxiliary_virt);
+
 	if (oob_required) {
 		/*
 		 * It's time to deliver the OOB bytes. See gpmi_ecc_read_oob()
-- 
2.11.0

[PATCH 02/10] mtd: nand: gpmi: Utilize hardware to detect bitflips in erased blocks

[Sascha Hauer]

The GPMI nand has a hardware feature to ignore bitflips in erased pages.
Use this feature rather than the longish code we have now.
Unfortunately the bitflips in erased pages are not corrected, so we have
to memset the read data before passing it to the upper layers.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 77 ++++------------------------------
 1 file changed, 9 insertions(+), 68 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index d4d824ef64e9..09e8ded3f1e2 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1056,6 +1056,8 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	auxiliary_virt = this->auxiliary_virt;
 	auxiliary_phys = this->auxiliary_phys;
 
+	writel(mtd->bitflip_threshold, this->resources.bch_regs + HW_BCH_MODE);
+
 	/* go! */
 	ret = gpmi_read_page(this, payload_phys, auxiliary_phys);
 	read_page_end(this, buf, nfc_geo->payload_size,
@@ -1076,77 +1078,16 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 			   payload_virt, payload_phys);
 
 	for (i = 0; i < nfc_geo->ecc_chunk_count; i++, status++) {
-		if ((*status == STATUS_GOOD) || (*status == STATUS_ERASED))
+		if (*status == STATUS_GOOD)
 			continue;
 
-		if (*status == STATUS_UNCORRECTABLE) {
-			int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
-			u8 *eccbuf = this->raw_buffer;
-			int offset, bitoffset;
-			int eccbytes;
-			int flips;
-
-			/* Read ECC bytes into our internal raw_buffer */
-			offset = nfc_geo->metadata_size * 8;
-			offset += ((8 * nfc_geo->ecc_chunk_size) + eccbits) * (i + 1);
-			offset -= eccbits;
-			bitoffset = offset % 8;
-			eccbytes = DIV_ROUND_UP(offset + eccbits, 8);
-			offset /= 8;
-			eccbytes -= offset;
-			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
-			chip->read_buf(mtd, eccbuf, eccbytes);
-
-			/*
-			 * ECC data are not byte aligned and we may have
-			 * in-band data in the first and last byte of
-			 * eccbuf. Set non-eccbits to one so that
-			 * nand_check_erased_ecc_chunk() does not count them
-			 * as bitflips.
-			 */
-			if (bitoffset)
-				eccbuf[0] |= GENMASK(bitoffset - 1, 0);
-
-			bitoffset = (bitoffset + eccbits) % 8;
-			if (bitoffset)
-				eccbuf[eccbytes - 1] |= GENMASK(7, bitoffset);
-
-			/*
-			 * The ECC hardware has an uncorrectable ECC status
-			 * code in case we have bitflips in an erased page. As
-			 * nothing was written into this subpage the ECC is
-			 * obviously wrong and we can not trust it. We assume
-			 * at this point that we are reading an erased page and
-			 * try to correct the bitflips in buffer up to
-			 * ecc_strength bitflips. If this is a page with random
-			 * data, we exceed this number of bitflips and have a
-			 * ECC failure. Otherwise we use the corrected buffer.
-			 */
-			if (i == 0) {
-				/* The first block includes metadata */
-				flips = nand_check_erased_ecc_chunk(
-						buf + i * nfc_geo->ecc_chunk_size,
-						nfc_geo->ecc_chunk_size,
-						eccbuf, eccbytes,
-						auxiliary_virt,
-						nfc_geo->metadata_size,
-						nfc_geo->ecc_strength);
-			} else {
-				flips = nand_check_erased_ecc_chunk(
-						buf + i * nfc_geo->ecc_chunk_size,
-						nfc_geo->ecc_chunk_size,
-						eccbuf, eccbytes,
-						NULL, 0,
-						nfc_geo->ecc_strength);
-			}
-
-			if (flips > 0) {
-				max_bitflips = max_t(unsigned int, max_bitflips,
-						     flips);
-				mtd->ecc_stats.corrected += flips;
-				continue;
-			}
+		if (*status == STATUS_ERASED) {
+			memset(buf + nfc_geo->ecc_chunk_size * i, 0xff,
+			       nfc_geo->ecc_chunk_size);
+			continue;
+		}
 
+		if (*status == STATUS_UNCORRECTABLE) {
 			mtd->ecc_stats.failed++;
 			continue;
 		}
-- 
2.11.0

[PATCH 03/10] mtd: nand: gpmi: drop dma_ops_type

[Sascha Hauer]

The GPMI nand driver puts dma_ops_type in its private data struct. Based
on the ops type the DMA callback handler unmaps previously mapped
buffers. Instead of unmapping the buffers in the DMA callback handler,
do this in the caller directly which waits for the DMA transfer to
finish. This makes the whole dma_ops_type mechanism unnecessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 37 ++++++++++++++++++++--------------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 31 ++--------------------------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 11 ----------
 3 files changed, 24 insertions(+), 55 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 97787246af41..f29349d1cb7a 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -1111,13 +1111,6 @@ int gpmi_is_ready(struct gpmi_nand_data *this, unsigned chip)
 	return reg & mask;
 }
 
-static inline void set_dma_type(struct gpmi_nand_data *this,
-					enum dma_ops_type type)
-{
-	this->last_dma_type = this->dma_type;
-	this->dma_type = type;
-}
-
 int gpmi_send_command(struct gpmi_nand_data *this)
 {
 	struct dma_chan *channel = get_dma_chan(this);
@@ -1153,7 +1146,6 @@ int gpmi_send_command(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [3] submit the DMA */
-	set_dma_type(this, DMA_FOR_COMMAND);
 	return start_dma_without_bch_irq(this, desc);
 }
 
@@ -1162,6 +1154,7 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
 	int chip = this->current_chip;
+	int ret;
 	uint32_t command_mode;
 	uint32_t address;
 	u32 pio[2];
@@ -1191,8 +1184,11 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [3] submit the DMA */
-	set_dma_type(this, DMA_FOR_WRITE_DATA);
-	return start_dma_without_bch_irq(this, desc);
+	ret = start_dma_without_bch_irq(this, desc);
+
+	dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_TO_DEVICE);
+
+	return ret;
 }
 
 int gpmi_read_data(struct gpmi_nand_data *this)
@@ -1200,6 +1196,7 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
 	int chip = this->current_chip;
+	int ret;
 	u32 pio[2];
 
 	/* [1] : send PIO */
@@ -1225,8 +1222,14 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [3] : submit the DMA */
-	set_dma_type(this, DMA_FOR_READ_DATA);
-	return start_dma_without_bch_irq(this, desc);
+
+	ret = start_dma_without_bch_irq(this, desc);
+
+	dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_FROM_DEVICE);
+	if (this->direct_dma_map_ok == false)
+		memcpy(this->upper_buf, this->data_buffer_dma, this->upper_len);
+
+	return ret;
 }
 
 int gpmi_send_page(struct gpmi_nand_data *this,
@@ -1270,7 +1273,6 @@ int gpmi_send_page(struct gpmi_nand_data *this,
 	if (!desc)
 		return -EINVAL;
 
-	set_dma_type(this, DMA_FOR_WRITE_ECC_PAGE);
 	return start_dma_with_bch_irq(this, desc);
 }
 
@@ -1285,6 +1287,7 @@ int gpmi_read_page(struct gpmi_nand_data *this,
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
 	int chip = this->current_chip;
+	int ret;
 	u32 pio[6];
 
 	/* [1] Wait for the chip to report ready. */
@@ -1352,8 +1355,12 @@ int gpmi_read_page(struct gpmi_nand_data *this,
 		return -EINVAL;
 
 	/* [4] submit the DMA */
-	set_dma_type(this, DMA_FOR_READ_ECC_PAGE);
-	return start_dma_with_bch_irq(this, desc);
+
+	ret = start_dma_with_bch_irq(this, desc);
+
+	dma_unmap_sg(this->dev, &this->cmd_sgl, 1, DMA_TO_DEVICE);
+
+	return ret;
 }
 
 /**
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 09e8ded3f1e2..da1090474b11 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -472,31 +472,6 @@ static void dma_irq_callback(void *param)
 	struct gpmi_nand_data *this = param;
 	struct completion *dma_c = &this->dma_done;
 
-	switch (this->dma_type) {
-	case DMA_FOR_COMMAND:
-		dma_unmap_sg(this->dev, &this->cmd_sgl, 1, DMA_TO_DEVICE);
-		break;
-
-	case DMA_FOR_READ_DATA:
-		dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_FROM_DEVICE);
-		if (this->direct_dma_map_ok == false)
-			memcpy(this->upper_buf, this->data_buffer_dma,
-				this->upper_len);
-		break;
-
-	case DMA_FOR_WRITE_DATA:
-		dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_TO_DEVICE);
-		break;
-
-	case DMA_FOR_READ_ECC_PAGE:
-	case DMA_FOR_WRITE_ECC_PAGE:
-		/* We have to wait the BCH interrupt to finish. */
-		break;
-
-	default:
-		dev_err(this->dev, "in wrong DMA operation.\n");
-	}
-
 	complete(dma_c);
 }
 
@@ -516,8 +491,7 @@ int start_dma_without_bch_irq(struct gpmi_nand_data *this,
 	/* Wait for the interrupt from the DMA block. */
 	timeout = wait_for_completion_timeout(dma_c, msecs_to_jiffies(1000));
 	if (!timeout) {
-		dev_err(this->dev, "DMA timeout, last DMA :%d\n",
-			this->last_dma_type);
+		dev_err(this->dev, "DMA timeout, last DMA\n");
 		gpmi_dump_info(this);
 		return -ETIMEDOUT;
 	}
@@ -546,8 +520,7 @@ int start_dma_with_bch_irq(struct gpmi_nand_data *this,
 	/* Wait for the interrupt from the BCH block. */
 	timeout = wait_for_completion_timeout(bch_c, msecs_to_jiffies(1000));
 	if (!timeout) {
-		dev_err(this->dev, "BCH timeout, last DMA :%d\n",
-			this->last_dma_type);
+		dev_err(this->dev, "BCH timeout\n");
 		gpmi_dump_info(this);
 		return -ETIMEDOUT;
 	}
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index a45e4ce13d10..602e47ce4e88 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -77,15 +77,6 @@ struct boot_rom_geometry {
 	unsigned int  search_area_stride_exponent;
 };
 
-/* DMA operations types */
-enum dma_ops_type {
-	DMA_FOR_COMMAND = 1,
-	DMA_FOR_READ_DATA,
-	DMA_FOR_WRITE_DATA,
-	DMA_FOR_READ_ECC_PAGE,
-	DMA_FOR_WRITE_ECC_PAGE
-};
-
 /**
  * struct nand_timing - Fundamental timing attributes for NAND.
  * @data_setup_in_ns:         The data setup time, in nanoseconds. Usually the
@@ -196,8 +187,6 @@ struct gpmi_nand_data {
 	/* DMA channels */
 #define DMA_CHANS		8
 	struct dma_chan		*dma_chans[DMA_CHANS];
-	enum dma_ops_type	last_dma_type;
-	enum dma_ops_type	dma_type;
 	struct completion	dma_done;
 
 	/* private */
-- 
2.11.0

[PATCH 04/10] mtd: nand: gpmi: pass buffer and len around

[Sascha Hauer]

Instead of putting the buffer and len passed in from the mtd core
into the private data struct, just pass it around in the GPMI
drivers functions. This makes the lifetime of the variables more
clear and the code easier to follow.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 14 +++++++-------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 20 ++++++++------------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 10 +++-------
 3 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index f29349d1cb7a..503b024aa883 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -1149,7 +1149,7 @@ int gpmi_send_command(struct gpmi_nand_data *this)
 	return start_dma_without_bch_irq(this, desc);
 }
 
-int gpmi_send_data(struct gpmi_nand_data *this)
+int gpmi_send_data(struct gpmi_nand_data *this, const void *buf, int len)
 {
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
@@ -1168,7 +1168,7 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 		| BF_GPMI_CTRL0_CS(chip, this)
 		| BF_GPMI_CTRL0_LOCK_CS(LOCK_CS_ENABLE, this)
 		| BF_GPMI_CTRL0_ADDRESS(address)
-		| BF_GPMI_CTRL0_XFER_COUNT(this->upper_len);
+		| BF_GPMI_CTRL0_XFER_COUNT(len);
 	pio[1] = 0;
 	desc = dmaengine_prep_slave_sg(channel, (struct scatterlist *)pio,
 					ARRAY_SIZE(pio), DMA_TRANS_NONE, 0);
@@ -1176,7 +1176,7 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [2] send DMA request */
-	prepare_data_dma(this, DMA_TO_DEVICE);
+	prepare_data_dma(this, buf, len, DMA_TO_DEVICE);
 	desc = dmaengine_prep_slave_sg(channel, &this->data_sgl,
 					1, DMA_MEM_TO_DEV,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -1191,7 +1191,7 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 	return ret;
 }
 
-int gpmi_read_data(struct gpmi_nand_data *this)
+int gpmi_read_data(struct gpmi_nand_data *this, void *buf, int len)
 {
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
@@ -1205,7 +1205,7 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 		| BF_GPMI_CTRL0_CS(chip, this)
 		| BF_GPMI_CTRL0_LOCK_CS(LOCK_CS_ENABLE, this)
 		| BF_GPMI_CTRL0_ADDRESS(BV_GPMI_CTRL0_ADDRESS__NAND_DATA)
-		| BF_GPMI_CTRL0_XFER_COUNT(this->upper_len);
+		| BF_GPMI_CTRL0_XFER_COUNT(len);
 	pio[1] = 0;
 	desc = dmaengine_prep_slave_sg(channel,
 					(struct scatterlist *)pio,
@@ -1214,7 +1214,7 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [2] : send DMA request */
-	prepare_data_dma(this, DMA_FROM_DEVICE);
+	prepare_data_dma(this, buf, len, DMA_FROM_DEVICE);
 	desc = dmaengine_prep_slave_sg(channel, &this->data_sgl,
 					1, DMA_DEV_TO_MEM,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -1227,7 +1227,7 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 
 	dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_FROM_DEVICE);
 	if (this->direct_dma_map_ok == false)
-		memcpy(this->upper_buf, this->data_buffer_dma, this->upper_len);
+		memcpy(buf, this->data_buffer_dma, len);
 
 	return ret;
 }
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index da1090474b11..0f525a45b7ec 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -437,15 +437,15 @@ struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
 }
 
 /* Can we use the upper's buffer directly for DMA? */
-void prepare_data_dma(struct gpmi_nand_data *this, enum dma_data_direction dr)
+void prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
+		      enum dma_data_direction dr)
 {
 	struct scatterlist *sgl = &this->data_sgl;
 	int ret;
 
 	/* first try to map the upper buffer directly */
-	if (virt_addr_valid(this->upper_buf) &&
-		!object_is_on_stack(this->upper_buf)) {
-		sg_init_one(sgl, this->upper_buf, this->upper_len);
+	if (virt_addr_valid(buf) && !object_is_on_stack(buf)) {
+		sg_init_one(sgl, buf, len);
 		ret = dma_map_sg(this->dev, sgl, 1, dr);
 		if (ret == 0)
 			goto map_fail;
@@ -456,10 +456,10 @@ void prepare_data_dma(struct gpmi_nand_data *this, enum dma_data_direction dr)
 
 map_fail:
 	/* We have to use our own DMA buffer. */
-	sg_init_one(sgl, this->data_buffer_dma, this->upper_len);
+	sg_init_one(sgl, this->data_buffer_dma, len);
 
 	if (dr == DMA_TO_DEVICE)
-		memcpy(this->data_buffer_dma, this->upper_buf, this->upper_len);
+		memcpy(this->data_buffer_dma, buf, len);
 
 	dma_map_sg(this->dev, sgl, 1, dr);
 
@@ -926,10 +926,8 @@ static void gpmi_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
 
 	dev_dbg(this->dev, "len is %d\n", len);
-	this->upper_buf	= buf;
-	this->upper_len	= len;
 
-	gpmi_read_data(this);
+	gpmi_read_data(this, buf, len);
 }
 
 static void gpmi_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
@@ -938,10 +936,8 @@ static void gpmi_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
 
 	dev_dbg(this->dev, "len is %d\n", len);
-	this->upper_buf	= (uint8_t *)buf;
-	this->upper_len	= len;
 
-	gpmi_send_data(this);
+	gpmi_send_data(this, buf, len);
 }
 
 static uint8_t gpmi_read_byte(struct mtd_info *mtd)
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 602e47ce4e88..a14bfda45156 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -159,10 +159,6 @@ struct gpmi_nand_data {
 	int			current_chip;
 	unsigned int		command_length;
 
-	/* passed from upper layer */
-	uint8_t			*upper_buf;
-	int			upper_len;
-
 	/* for DMA operations */
 	bool			direct_dma_map_ok;
 
@@ -259,7 +255,7 @@ struct timing_threshold {
 /* Common Services */
 extern int common_nfc_set_geometry(struct gpmi_nand_data *);
 extern struct dma_chan *get_dma_chan(struct gpmi_nand_data *);
-extern void prepare_data_dma(struct gpmi_nand_data *,
+extern void prepare_data_dma(struct gpmi_nand_data *, const void *buf, int len,
 				enum dma_data_direction dr);
 extern int start_dma_without_bch_irq(struct gpmi_nand_data *,
 				struct dma_async_tx_descriptor *);
@@ -276,8 +272,8 @@ extern int gpmi_is_ready(struct gpmi_nand_data *, unsigned chip);
 extern int gpmi_send_command(struct gpmi_nand_data *);
 extern void gpmi_begin(struct gpmi_nand_data *);
 extern void gpmi_end(struct gpmi_nand_data *);
-extern int gpmi_read_data(struct gpmi_nand_data *);
-extern int gpmi_send_data(struct gpmi_nand_data *);
+extern int gpmi_read_data(struct gpmi_nand_data *, void *buf, int len);
+extern int gpmi_send_data(struct gpmi_nand_data *, const void *buf, int len);
 extern int gpmi_send_page(struct gpmi_nand_data *,
 			dma_addr_t payload, dma_addr_t auxiliary);
 extern int gpmi_read_page(struct gpmi_nand_data *,
-- 
2.11.0

[PATCH 05/10] mtd: nand: gpmi: put only once used functions inline

[Sascha Hauer]

read_page_prepare(), read_page_end() and read_page_swap_end() are
trivial functions that are used only once and take 8 arguments each.
De-obfuscate the code by open coding these functions in
gpmi_ecc_read_page()

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 89 +++++++++-------------------------
 1 file changed, 23 insertions(+), 66 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 0f525a45b7ec..db6664ef7c90 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -696,56 +696,6 @@ static int init_hardware(struct gpmi_nand_data *this)
 	return 0;
 }
 
-static int read_page_prepare(struct gpmi_nand_data *this,
-			void *destination, unsigned length,
-			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
-			void **use_virt, dma_addr_t *use_phys)
-{
-	struct device *dev = this->dev;
-
-	if (virt_addr_valid(destination)) {
-		dma_addr_t dest_phys;
-
-		dest_phys = dma_map_single(dev, destination,
-						length, DMA_FROM_DEVICE);
-		if (dma_mapping_error(dev, dest_phys)) {
-			if (alt_size < length) {
-				dev_err(dev, "Alternate buffer is too small\n");
-				return -ENOMEM;
-			}
-			goto map_failed;
-		}
-		*use_virt = destination;
-		*use_phys = dest_phys;
-		this->direct_dma_map_ok = true;
-		return 0;
-	}
-
-map_failed:
-	*use_virt = alt_virt;
-	*use_phys = alt_phys;
-	this->direct_dma_map_ok = false;
-	return 0;
-}
-
-static inline void read_page_end(struct gpmi_nand_data *this,
-			void *destination, unsigned length,
-			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
-			void *used_virt, dma_addr_t used_phys)
-{
-	if (this->direct_dma_map_ok)
-		dma_unmap_single(this->dev, used_phys, length, DMA_FROM_DEVICE);
-}
-
-static inline void read_page_swap_end(struct gpmi_nand_data *this,
-			void *destination, unsigned length,
-			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
-			void *used_virt, dma_addr_t used_phys)
-{
-	if (!this->direct_dma_map_ok)
-		memcpy(destination, alt_virt, length);
-}
-
 static int send_page_prepare(struct gpmi_nand_data *this,
 			const void *source, unsigned length,
 			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
@@ -1013,15 +963,23 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	int           ret;
 
 	dev_dbg(this->dev, "page number is : %d\n", page);
-	ret = read_page_prepare(this, buf, nfc_geo->payload_size,
-					this->payload_virt, this->payload_phys,
-					nfc_geo->payload_size,
-					&payload_virt, &payload_phys);
-	if (ret) {
-		dev_err(this->dev, "Inadequate DMA buffer\n");
-		ret = -ENOMEM;
-		return ret;
+
+	payload_virt = this->payload_virt;
+	payload_phys = this->payload_phys;
+	this->direct_dma_map_ok = false;
+
+	if (virt_addr_valid(buf)) {
+		dma_addr_t dest_phys;
+
+		dest_phys = dma_map_single(this->dev, buf, nfc_geo->payload_size,
+					   DMA_FROM_DEVICE);
+		if (!dma_mapping_error(this->dev, dest_phys)) {
+			payload_virt = buf;
+			payload_phys = dest_phys;
+			this->direct_dma_map_ok = true;
+		}
 	}
+
 	auxiliary_virt = this->auxiliary_virt;
 	auxiliary_phys = this->auxiliary_phys;
 
@@ -1029,10 +987,11 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 
 	/* go! */
 	ret = gpmi_read_page(this, payload_phys, auxiliary_phys);
-	read_page_end(this, buf, nfc_geo->payload_size,
-			this->payload_virt, this->payload_phys,
-			nfc_geo->payload_size,
-			payload_virt, payload_phys);
+
+	if (this->direct_dma_map_ok)
+		dma_unmap_single(this->dev, payload_phys, nfc_geo->payload_size,
+				 DMA_FROM_DEVICE);
+
 	if (ret) {
 		dev_err(this->dev, "Error in ECC-based read: %d\n", ret);
 		return ret;
@@ -1041,10 +1000,8 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	/* Loop over status bytes, accumulating ECC status. */
 	status = auxiliary_virt + nfc_geo->auxiliary_status_offset;
 
-	read_page_swap_end(this, buf, nfc_geo->payload_size,
-			   this->payload_virt, this->payload_phys,
-			   nfc_geo->payload_size,
-			   payload_virt, payload_phys);
+	if (!this->direct_dma_map_ok)
+		memcpy(buf, this->payload_virt, nfc_geo->payload_size);
 
 	for (i = 0; i < nfc_geo->ecc_chunk_count; i++, status++) {
 		if (*status == STATUS_GOOD)
-- 
2.11.0

[PATCH 06/10] mtd: nand: gpmi: remove direct_dma_map_ok from driver data struct

[Sascha Hauer]

Instead of putting direct_dma_map_ok into driver struct pass it around
between functions to make the code more readable.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |  5 +++--
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++--------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  5 +----
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 503b024aa883..83996f7783ab 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -1198,6 +1198,7 @@ int gpmi_read_data(struct gpmi_nand_data *this, void *buf, int len)
 	int chip = this->current_chip;
 	int ret;
 	u32 pio[2];
+	bool direct;
 
 	/* [1] : send PIO */
 	pio[0] = BF_GPMI_CTRL0_COMMAND_MODE(BV_GPMI_CTRL0_COMMAND_MODE__READ)
@@ -1214,7 +1215,7 @@ int gpmi_read_data(struct gpmi_nand_data *this, void *buf, int len)
 		return -EINVAL;
 
 	/* [2] : send DMA request */
-	prepare_data_dma(this, buf, len, DMA_FROM_DEVICE);
+	direct = prepare_data_dma(this, buf, len, DMA_FROM_DEVICE);
 	desc = dmaengine_prep_slave_sg(channel, &this->data_sgl,
 					1, DMA_DEV_TO_MEM,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -1226,7 +1227,7 @@ int gpmi_read_data(struct gpmi_nand_data *this, void *buf, int len)
 	ret = start_dma_without_bch_irq(this, desc);
 
 	dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_FROM_DEVICE);
-	if (this->direct_dma_map_ok == false)
+	if (!direct)
 		memcpy(buf, this->data_buffer_dma, len);
 
 	return ret;
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index db6664ef7c90..09eb1960a60d 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -437,7 +437,7 @@ struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
 }
 
 /* Can we use the upper's buffer directly for DMA? */
-void prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
+bool prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
 		      enum dma_data_direction dr)
 {
 	struct scatterlist *sgl = &this->data_sgl;
@@ -450,8 +450,7 @@ void prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
 		if (ret == 0)
 			goto map_fail;
 
-		this->direct_dma_map_ok = true;
-		return;
+		return true;
 	}
 
 map_fail:
@@ -463,7 +462,7 @@ void prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
 
 	dma_map_sg(this->dev, sgl, 1, dr);
 
-	this->direct_dma_map_ok = false;
+	return false;
 }
 
 /* This will be called after the DMA operation is finished. */
@@ -961,12 +960,12 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	unsigned char *status;
 	unsigned int  max_bitflips = 0;
 	int           ret;
+	bool          direct = false;
 
 	dev_dbg(this->dev, "page number is : %d\n", page);
 
 	payload_virt = this->payload_virt;
 	payload_phys = this->payload_phys;
-	this->direct_dma_map_ok = false;
 
 	if (virt_addr_valid(buf)) {
 		dma_addr_t dest_phys;
@@ -976,7 +975,7 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 		if (!dma_mapping_error(this->dev, dest_phys)) {
 			payload_virt = buf;
 			payload_phys = dest_phys;
-			this->direct_dma_map_ok = true;
+			direct = true;
 		}
 	}
 
@@ -988,7 +987,7 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	/* go! */
 	ret = gpmi_read_page(this, payload_phys, auxiliary_phys);
 
-	if (this->direct_dma_map_ok)
+	if (direct)
 		dma_unmap_single(this->dev, payload_phys, nfc_geo->payload_size,
 				 DMA_FROM_DEVICE);
 
@@ -1000,7 +999,7 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	/* Loop over status bytes, accumulating ECC status. */
 	status = auxiliary_virt + nfc_geo->auxiliary_status_offset;
 
-	if (!this->direct_dma_map_ok)
+	if (!direct)
 		memcpy(buf, this->payload_virt, nfc_geo->payload_size);
 
 	for (i = 0; i < nfc_geo->ecc_chunk_count; i++, status++) {
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index a14bfda45156..769ff91a714f 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -159,9 +159,6 @@ struct gpmi_nand_data {
 	int			current_chip;
 	unsigned int		command_length;
 
-	/* for DMA operations */
-	bool			direct_dma_map_ok;
-
 	struct scatterlist	cmd_sgl;
 	char			*cmd_buffer;
 
@@ -255,7 +252,7 @@ struct timing_threshold {
 /* Common Services */
 extern int common_nfc_set_geometry(struct gpmi_nand_data *);
 extern struct dma_chan *get_dma_chan(struct gpmi_nand_data *);
-extern void prepare_data_dma(struct gpmi_nand_data *, const void *buf, int len,
+extern bool prepare_data_dma(struct gpmi_nand_data *, const void *buf, int len,
 				enum dma_data_direction dr);
 extern int start_dma_without_bch_irq(struct gpmi_nand_data *,
 				struct dma_async_tx_descriptor *);
-- 
2.11.0

[PATCH 07/10] mtd: nand: gpmi: return valid value from bch_set_geometry()

[Sascha Hauer]

The caller of bch_set_geometry() expects the return value to
be an error code, so !0 is not valid. return the error from the
just called function instead.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 83996f7783ab..ae3adbf4fe42 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -259,8 +259,9 @@ int bch_set_geometry(struct gpmi_nand_data *this)
 	unsigned int gf_len;
 	int ret;
 
-	if (common_nfc_set_geometry(this))
-		return !0;
+	ret = common_nfc_set_geometry(this);
+	if (ret)
+		return ret;
 
 	block_count   = bch_geo->ecc_chunk_count - 1;
 	block_size    = bch_geo->ecc_chunk_size;
-- 
2.11.0

[PATCH 08/10] mtd: nand: gpmi: remove unnecessary variables

[Sascha Hauer]

Use this->auxiliary_virt and this->auxiliary_phys directly rather
than creating extra local variables for them.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 09eb1960a60d..8dc40b672ab9 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -954,8 +954,6 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	struct bch_geometry *nfc_geo = &this->bch_geometry;
 	void          *payload_virt;
 	dma_addr_t    payload_phys;
-	void          *auxiliary_virt;
-	dma_addr_t    auxiliary_phys;
 	unsigned int  i;
 	unsigned char *status;
 	unsigned int  max_bitflips = 0;
@@ -979,13 +977,10 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 		}
 	}
 
-	auxiliary_virt = this->auxiliary_virt;
-	auxiliary_phys = this->auxiliary_phys;
-
 	writel(mtd->bitflip_threshold, this->resources.bch_regs + HW_BCH_MODE);
 
 	/* go! */
-	ret = gpmi_read_page(this, payload_phys, auxiliary_phys);
+	ret = gpmi_read_page(this, payload_phys, this->auxiliary_phys);
 
 	if (direct)
 		dma_unmap_single(this->dev, payload_phys, nfc_geo->payload_size,
@@ -997,7 +992,7 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	}
 
 	/* Loop over status bytes, accumulating ECC status. */
-	status = auxiliary_virt + nfc_geo->auxiliary_status_offset;
+	status = this->auxiliary_virt + nfc_geo->auxiliary_status_offset;
 
 	if (!direct)
 		memcpy(buf, this->payload_virt, nfc_geo->payload_size);
@@ -1022,7 +1017,7 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	}
 
 	/* handle the block mark swapping */
-	block_mark_swapping(this, buf, auxiliary_virt);
+	block_mark_swapping(this, buf, this->auxiliary_virt);
 
 	if (oob_required) {
 		/*
@@ -1036,7 +1031,7 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 		 * the block mark.
 		 */
 		memset(chip->oob_poi, ~0, mtd->oobsize);
-		chip->oob_poi[0] = ((uint8_t *) auxiliary_virt)[0];
+		chip->oob_poi[0] = ((uint8_t *)this->auxiliary_virt)[0];
 	}
 
 	return max_bitflips;
-- 
2.11.0

[PATCH 09/10] mtd: nand: gpmi: rework gpmi_ecc_write_page

[Sascha Hauer]

gpmi_ecc_write_page() used to call several functions with 8 arguments
each. refactor this to make it easier to follow.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 140 ++++++++++-----------------------
 1 file changed, 42 insertions(+), 98 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 8dc40b672ab9..d8038b62246c 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -695,51 +695,6 @@ static int init_hardware(struct gpmi_nand_data *this)
 	return 0;
 }
 
-static int send_page_prepare(struct gpmi_nand_data *this,
-			const void *source, unsigned length,
-			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
-			const void **use_virt, dma_addr_t *use_phys)
-{
-	struct device *dev = this->dev;
-
-	if (virt_addr_valid(source)) {
-		dma_addr_t source_phys;
-
-		source_phys = dma_map_single(dev, (void *)source, length,
-						DMA_TO_DEVICE);
-		if (dma_mapping_error(dev, source_phys)) {
-			if (alt_size < length) {
-				dev_err(dev, "Alternate buffer is too small\n");
-				return -ENOMEM;
-			}
-			goto map_failed;
-		}
-		*use_virt = source;
-		*use_phys = source_phys;
-		return 0;
-	}
-map_failed:
-	/*
-	 * Copy the content of the source buffer into the alternate
-	 * buffer and set up the return values accordingly.
-	 */
-	memcpy(alt_virt, source, length);
-
-	*use_virt = alt_virt;
-	*use_phys = alt_phys;
-	return 0;
-}
-
-static void send_page_end(struct gpmi_nand_data *this,
-			const void *source, unsigned length,
-			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
-			const void *used_virt, dma_addr_t used_phys)
-{
-	struct device *dev = this->dev;
-	if (used_virt == source)
-		dma_unmap_single(dev, used_phys, length, DMA_TO_DEVICE);
-}
-
 static void gpmi_free_dma_buffer(struct gpmi_nand_data *this)
 {
 	struct device *dev = this->dev;
@@ -1126,60 +1081,53 @@ static int gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	return max_bitflips;
 }
 
+
+static bool map_write_direct(struct gpmi_nand_data *this, const void *source,
+		       unsigned length, dma_addr_t *phys)
+{
+	struct device *dev = this->dev;
+	dma_addr_t source_phys;
+
+	/*
+	 * input data is const, we cannot map directly when doing bad block
+	 * marker swapping.
+	 */
+	if (this->swap_block_mark)
+		return false;
+
+	if (!virt_addr_valid(source))
+		return false;
+
+	source_phys = dma_map_single(dev, (void *)source, length,
+					DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, source_phys))
+		return false;
+
+	return true;
+}
+
 static int gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 				const uint8_t *buf, int oob_required, int page)
 {
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
-	struct bch_geometry *nfc_geo = &this->bch_geometry;
-	const void *payload_virt;
 	dma_addr_t payload_phys;
-	const void *auxiliary_virt;
 	dma_addr_t auxiliary_phys;
-	int        ret;
+	int ret;
+	bool user_mapped, oob_mapped;
 
 	dev_dbg(this->dev, "ecc write page.\n");
-	if (this->swap_block_mark) {
-		/*
-		 * If control arrives here, we're doing block mark swapping.
-		 * Since we can't modify the caller's buffers, we must copy them
-		 * into our own.
-		 */
+
+	user_mapped = map_write_direct(this, buf, mtd->writesize, &payload_phys);
+	if (!user_mapped) {
 		memcpy(this->payload_virt, buf, mtd->writesize);
-		payload_virt = this->payload_virt;
 		payload_phys = this->payload_phys;
+	}
 
-		memcpy(this->auxiliary_virt, chip->oob_poi,
-				nfc_geo->auxiliary_size);
-		auxiliary_virt = this->auxiliary_virt;
+	oob_mapped = map_write_direct(this, chip->oob_poi, mtd->oobsize,
+				&auxiliary_phys);
+	if (!oob_mapped) {
+		memcpy(this->auxiliary_virt, buf, mtd->oobsize);
 		auxiliary_phys = this->auxiliary_phys;
-
-		/* Handle block mark swapping. */
-		block_mark_swapping(this,
-				(void *)payload_virt, (void *)auxiliary_virt);
-	} else {
-		/*
-		 * If control arrives here, we're not doing block mark swapping,
-		 * so we can to try and use the caller's buffers.
-		 */
-		ret = send_page_prepare(this,
-				buf, mtd->writesize,
-				this->payload_virt, this->payload_phys,
-				nfc_geo->payload_size,
-				&payload_virt, &payload_phys);
-		if (ret) {
-			dev_err(this->dev, "Inadequate payload DMA buffer\n");
-			return 0;
-		}
-
-		ret = send_page_prepare(this,
-				chip->oob_poi, mtd->oobsize,
-				this->auxiliary_virt, this->auxiliary_phys,
-				nfc_geo->auxiliary_size,
-				&auxiliary_virt, &auxiliary_phys);
-		if (ret) {
-			dev_err(this->dev, "Inadequate auxiliary DMA buffer\n");
-			goto exit_auxiliary;
-		}
 	}
 
 	/* Ask the NFC. */
@@ -1187,17 +1135,13 @@ static int gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 	if (ret)
 		dev_err(this->dev, "Error in ECC-based write: %d\n", ret);
 
-	if (!this->swap_block_mark) {
-		send_page_end(this, chip->oob_poi, mtd->oobsize,
-				this->auxiliary_virt, this->auxiliary_phys,
-				nfc_geo->auxiliary_size,
-				auxiliary_virt, auxiliary_phys);
-exit_auxiliary:
-		send_page_end(this, buf, mtd->writesize,
-				this->payload_virt, this->payload_phys,
-				nfc_geo->payload_size,
-				payload_virt, payload_phys);
-	}
+	if (oob_mapped)
+		dma_unmap_single(this->dev, auxiliary_phys, mtd->oobsize,
+				 DMA_TO_DEVICE);
+
+	if (user_mapped)
+		dma_unmap_single(this->dev, payload_phys, mtd->writesize,
+				 DMA_TO_DEVICE);
 
 	return 0;
 }
-- 
2.11.0

[PATCH 10/10] mtd: nand: gpmi: return error code from gpmi_ecc_write_page

[Sascha Hauer]

When something goes wrong in gpmi_ecc_write_page() return an
error rather than howling in the logs.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index d8038b62246c..d1f1bd69f496 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1132,8 +1132,6 @@ static int gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 
 	/* Ask the NFC. */
 	ret = gpmi_send_page(this, payload_phys, auxiliary_phys);
-	if (ret)
-		dev_err(this->dev, "Error in ECC-based write: %d\n", ret);
 
 	if (oob_mapped)
 		dma_unmap_single(this->dev, auxiliary_phys, mtd->oobsize,
@@ -1143,7 +1141,7 @@ static int gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 		dma_unmap_single(this->dev, payload_phys, mtd->writesize,
 				 DMA_TO_DEVICE);
 
-	return 0;
+	return ret;
 }
 
 /*
-- 
2.11.0

Re: [PATCH 02/10] mtd: nand: gpmi: Utilize hardware to detect bitflips in erased blocks

[Boris Brezillon]

Hi Sascha,

On Wed,  6 Dec 2017 10:19:17 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> The GPMI nand has a hardware feature to ignore bitflips in erased pages.
> Use this feature rather than the longish code we have now.
> Unfortunately the bitflips in erased pages are not corrected, so we have
> to memset the read data before passing it to the upper layers.

There's a good reason we didn't use the HW bitflip detection in the
first place: we currently have no way to report the number of corrected
bitflips in an erased page, and that's a big problem, because then UBI
does not know when it should re-erase the block.

Maybe we missed something when the initial proposal was done and
there's actually a way to retrieve this information, but if that's not
the case, I'd prefer to keep the existing implementation even if it's
slower and more verbose.

Regards,

Boris

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 77 ++++------------------------------
>  1 file changed, 9 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index d4d824ef64e9..09e8ded3f1e2 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1056,6 +1056,8 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	auxiliary_virt = this->auxiliary_virt;
>  	auxiliary_phys = this->auxiliary_phys;
>  
> +	writel(mtd->bitflip_threshold, this->resources.bch_regs + HW_BCH_MODE);
> +
>  	/* go! */
>  	ret = gpmi_read_page(this, payload_phys, auxiliary_phys);
>  	read_page_end(this, buf, nfc_geo->payload_size,
> @@ -1076,77 +1078,16 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  			   payload_virt, payload_phys);
>  
>  	for (i = 0; i < nfc_geo->ecc_chunk_count; i++, status++) {
> -		if ((*status == STATUS_GOOD) || (*status == STATUS_ERASED))
> +		if (*status == STATUS_GOOD)
>  			continue;
>  
> -		if (*status == STATUS_UNCORRECTABLE) {
> -			int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
> -			u8 *eccbuf = this->raw_buffer;
> -			int offset, bitoffset;
> -			int eccbytes;
> -			int flips;
> -
> -			/* Read ECC bytes into our internal raw_buffer */
> -			offset = nfc_geo->metadata_size * 8;
> -			offset += ((8 * nfc_geo->ecc_chunk_size) + eccbits) * (i + 1);
> -			offset -= eccbits;
> -			bitoffset = offset % 8;
> -			eccbytes = DIV_ROUND_UP(offset + eccbits, 8);
> -			offset /= 8;
> -			eccbytes -= offset;
> -			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
> -			chip->read_buf(mtd, eccbuf, eccbytes);
> -
> -			/*
> -			 * ECC data are not byte aligned and we may have
> -			 * in-band data in the first and last byte of
> -			 * eccbuf. Set non-eccbits to one so that
> -			 * nand_check_erased_ecc_chunk() does not count them
> -			 * as bitflips.
> -			 */
> -			if (bitoffset)
> -				eccbuf[0] |= GENMASK(bitoffset - 1, 0);
> -
> -			bitoffset = (bitoffset + eccbits) % 8;
> -			if (bitoffset)
> -				eccbuf[eccbytes - 1] |= GENMASK(7, bitoffset);
> -
> -			/*
> -			 * The ECC hardware has an uncorrectable ECC status
> -			 * code in case we have bitflips in an erased page. As
> -			 * nothing was written into this subpage the ECC is
> -			 * obviously wrong and we can not trust it. We assume
> -			 * at this point that we are reading an erased page and
> -			 * try to correct the bitflips in buffer up to
> -			 * ecc_strength bitflips. If this is a page with random
> -			 * data, we exceed this number of bitflips and have a
> -			 * ECC failure. Otherwise we use the corrected buffer.
> -			 */
> -			if (i == 0) {
> -				/* The first block includes metadata */
> -				flips = nand_check_erased_ecc_chunk(
> -						buf + i * nfc_geo->ecc_chunk_size,
> -						nfc_geo->ecc_chunk_size,
> -						eccbuf, eccbytes,
> -						auxiliary_virt,
> -						nfc_geo->metadata_size,
> -						nfc_geo->ecc_strength);
> -			} else {
> -				flips = nand_check_erased_ecc_chunk(
> -						buf + i * nfc_geo->ecc_chunk_size,
> -						nfc_geo->ecc_chunk_size,
> -						eccbuf, eccbytes,
> -						NULL, 0,
> -						nfc_geo->ecc_strength);
> -			}
> -
> -			if (flips > 0) {
> -				max_bitflips = max_t(unsigned int, max_bitflips,
> -						     flips);
> -				mtd->ecc_stats.corrected += flips;
> -				continue;
> -			}
> +		if (*status == STATUS_ERASED) {
> +			memset(buf + nfc_geo->ecc_chunk_size * i, 0xff,
> +			       nfc_geo->ecc_chunk_size);
> +			continue;
> +		}
>  
> +		if (*status == STATUS_UNCORRECTABLE) {
>  			mtd->ecc_stats.failed++;
>  			continue;
>  		}

Re: [PATCH 09/10] mtd: nand: gpmi: rework gpmi_ecc_write_page

[Sascha Hauer]

On Wed, Dec 06, 2017 at 10:19:24AM +0100, Sascha Hauer wrote:
> gpmi_ecc_write_page() used to call several functions with 8 arguments
> each. refactor this to make it easier to follow.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 140 ++++++++++-----------------------
>  1 file changed, 42 insertions(+), 98 deletions(-)

This patch is rubbish. Doesn't work as expected and needs update.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 02/10] mtd: nand: gpmi: Utilize hardware to detect bitflips in erased blocks

[Sascha Hauer]

On Wed, Dec 06, 2017 at 10:27:13AM +0100, Boris Brezillon wrote:
> Hi Sascha,
> 
> On Wed,  6 Dec 2017 10:19:17 +0100
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > The GPMI nand has a hardware feature to ignore bitflips in erased pages.
> > Use this feature rather than the longish code we have now.
> > Unfortunately the bitflips in erased pages are not corrected, so we have
> > to memset the read data before passing it to the upper layers.
> 
> There's a good reason we didn't use the HW bitflip detection in the
> first place: we currently have no way to report the number of corrected
> bitflips in an erased page, and that's a big problem, because then UBI
> does not know when it should re-erase the block.

Ah, ok.

> 
> Maybe we missed something when the initial proposal was done and
> there's actually a way to retrieve this information, but if that's not
> the case, I'd prefer to keep the existing implementation even if it's
> slower and more verbose.

I'm not so much concerned about the verbosity of the code but rather
about the magic it has inside. I have stared at this code for some time
now and still only vaguely know what it does.

We could do a bit better: We can still detect the number of bitflips
using nand_check_erased_ecc_chunk() without reading the oob data.
That would not be 100% accurate since we do not take the oob data into
account which might have bitflips aswell, but still should be good
enough, no?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 02/10] mtd: nand: gpmi: Utilize hardware to detect bitflips in erased blocks

[Boris Brezillon]

On Wed, 6 Dec 2017 16:28:04 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Wed, Dec 06, 2017 at 10:27:13AM +0100, Boris Brezillon wrote:
> > Hi Sascha,
> > 
> > On Wed,  6 Dec 2017 10:19:17 +0100
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > The GPMI nand has a hardware feature to ignore bitflips in erased pages.
> > > Use this feature rather than the longish code we have now.
> > > Unfortunately the bitflips in erased pages are not corrected, so we have
> > > to memset the read data before passing it to the upper layers.  
> > 
> > There's a good reason we didn't use the HW bitflip detection in the
> > first place: we currently have no way to report the number of corrected
> > bitflips in an erased page, and that's a big problem, because then UBI
> > does not know when it should re-erase the block.  
> 
> Ah, ok.
> 
> > 
> > Maybe we missed something when the initial proposal was done and
> > there's actually a way to retrieve this information, but if that's not
> > the case, I'd prefer to keep the existing implementation even if it's
> > slower and more verbose.  
> 
> I'm not so much concerned about the verbosity of the code but rather
> about the magic it has inside. I have stared at this code for some time
> now and still only vaguely know what it does.
> 
> We could do a bit better: We can still detect the number of bitflips
> using nand_check_erased_ecc_chunk() without reading the oob data.
> That would not be 100% accurate since we do not take the oob data into
> account which might have bitflips aswell, but still should be good
> enough, no?

Nope, we really have to take OOB bytes into account when counting
bitflips. Say that you have almost all in-band data set to 0xff, but
all OOB bytes reserved for ECC are set to 0 because someone decided to
write this portion of the page in raw mode. In this case we can't
consider the page as empty, otherwise we'll fail to correctly write ECC
bytes next time we program the page.

> 
> Sascha
> 

Re: [PATCH 02/10] mtd: nand: gpmi: Utilize hardware to detect bitflips in erased blocks

[Sascha Hauer]

On Wed, Dec 06, 2017 at 04:34:31PM +0100, Boris Brezillon wrote:
> On Wed, 6 Dec 2017 16:28:04 +0100
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Wed, Dec 06, 2017 at 10:27:13AM +0100, Boris Brezillon wrote:
> > > Hi Sascha,
> > > 
> > > On Wed,  6 Dec 2017 10:19:17 +0100
> > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >   
> > > > The GPMI nand has a hardware feature to ignore bitflips in erased pages.
> > > > Use this feature rather than the longish code we have now.
> > > > Unfortunately the bitflips in erased pages are not corrected, so we have
> > > > to memset the read data before passing it to the upper layers.  
> > > 
> > > There's a good reason we didn't use the HW bitflip detection in the
> > > first place: we currently have no way to report the number of corrected
> > > bitflips in an erased page, and that's a big problem, because then UBI
> > > does not know when it should re-erase the block.  
> > 
> > Ah, ok.
> > 
> > > 
> > > Maybe we missed something when the initial proposal was done and
> > > there's actually a way to retrieve this information, but if that's not
> > > the case, I'd prefer to keep the existing implementation even if it's
> > > slower and more verbose.  
> > 
> > I'm not so much concerned about the verbosity of the code but rather
> > about the magic it has inside. I have stared at this code for some time
> > now and still only vaguely know what it does.
> > 
> > We could do a bit better: We can still detect the number of bitflips
> > using nand_check_erased_ecc_chunk() without reading the oob data.
> > That would not be 100% accurate since we do not take the oob data into
> > account which might have bitflips aswell, but still should be good
> > enough, no?
> 
> Nope, we really have to take OOB bytes into account when counting
> bitflips. Say that you have almost all in-band data set to 0xff, but
> all OOB bytes reserved for ECC are set to 0 because someone decided to
> write this portion of the page in raw mode. In this case we can't
> consider the page as empty, otherwise we'll fail to correctly write ECC
> bytes next time we program the page.

I would probably be with you if at least the current code worked correctly,
but unfortunately it doesn't.

I did a test with the attached program. What it does is that it writes
pages nearly full of 0xff in raw mode. In the first page the first byte
is exchanged with a bitflip, in the second page the second byte is
exchanged with a bitflip, and so on. What I would expect that the number
of corrected bits is the same for all bitflip positions. What I get
instead is this:

./a.out /dev/mtd4 0x0f
byteno: 0x00000000 corrected: 5 failed: 0
byteno: 0x00000001 corrected: 4 failed: 0
byteno: 0x00000002 corrected: 5 failed: 0
byteno: 0x00000003 corrected: 6 failed: 0
byteno: 0x00000004 corrected: 5 failed: 0
byteno: 0x00000005 corrected: 5 failed: 0
byteno: 0x00000006 corrected: 4 failed: 0
byteno: 0x00000007 corrected: 5 failed: 0
byteno: 0x00000008 corrected: 5 failed: 0
byteno: 0x00000009 corrected: 4 failed: 0
...

(Read this as: byteno <x> in page has 0x0f instead of 0xff, so number
of corrected bits should be 4, instead we have 5)

or:
./a.out /dev/mtd4 0xfe
byteno: 0x00000000 corrected: 1 failed: 0
byteno: 0x00000001 corrected: 1 failed: 0
byteno: 0x00000002 corrected: 1 failed: 0
byteno: 0x00000003 corrected: 1 failed: 0
byteno: 0x00000004 corrected: 1 failed: 0
byteno: 0x00000005 corrected: 1 failed: 0
byteno: 0x00000006 corrected: 1 failed: 0
byteno: 0x00000007 corrected: 1 failed: 0
byteno: 0x00000008 corrected: 1 failed: 0
byteno: 0x00000009 corrected: 1 failed: 0
byteno: 0x0000000a corrected: 1 failed: 0
byteno: 0x0000000b corrected: 1 failed: 0
byteno: 0x0000000c corrected: 1 failed: 0
byteno: 0x0000000d corrected: 1 failed: 0
byteno: 0x0000000e corrected: 2 failed: 0
byteno: 0x0000000f corrected: 1 failed: 0
byteno: 0x00000010 corrected: 1 failed: 0
byteno: 0x00000011 corrected: 1 failed: 0
byteno: 0x00000012 corrected: 2 failed: 0
byteno: 0x00000013 corrected: 1 failed: 0

When I add a print_hex_dump to the driver I really see a buffer
with 5 bytes flipped instead of 4. When I do a raw read of the
page (under barebox, the Linux driver doesn't print the OOB data
properly), then I really see what I have written: A page with four
bitflips. The results are perfectly reproducable, so I don't expect
that to be any random read failures.

I know next to nothing about BCH, but could it be that the BCH engine
already starts correcting the page while it still doesn't know that
it can't be corrected at all leaving spurious cleared bits in the
read data?

Sascha

-------------------------------8<-------------------------------

#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <mtd/mtd-abi.h>
#include <sys/ioctl.h>
#include <stdint.h>
#include <string.h>

int fd;
uint8_t *buf;
uint8_t byte;

void erase_block(void)
{
	struct erase_info_user req = {
		.start = 0,
		.length = 128 * 1024,
	};
	int ret;

	ret = ioctl(fd, MEMERASE, &req);
	if (ret != 0) {
		perror("ioctl");
		exit (1);
	}
}

void get_stats(unsigned int *__corrected, unsigned int *__failed)
{
	struct mtd_ecc_stats stats;
	static unsigned int corrected, failed;
	int ret;

	ret = ioctl(fd, ECCGETSTATS, &stats);
	if (ret != 0) {
		perror("ioctl");
		exit (1);
 	}

	*__corrected = stats.corrected - corrected;
	*__failed = stats.failed - failed;

	corrected = stats.corrected;
	failed = stats.failed;
}

void wr_pages(int startbyte)
{
	struct mtd_write_req req = {
		.start = 0,
		.len = 2048,
		.ooblen = 64,
		.usr_data = (__u64)(unsigned long)buf,
		.usr_oob = (__u64)(unsigned long)(buf + 2048),
		.mode = MTD_OPS_RAW,
	};
	unsigned int corrected, failed;
	int i, ret;

	erase_block();

	for (i = 0; i < 64; i++) {
		memset(buf, 0xff, 2112);

		buf[i + startbyte] = byte;

		req.start = i * 2048;

		ret = ioctl(fd, MEMWRITE, &req);
		if (ret != 0) {
			perror("ioctl");
			exit(1);
		}
	}

	lseek(fd, 0, SEEK_SET);

	for (i = 0; i < 64; i++) {
		int j;

		ret = read(fd, buf, 2048);
		if (ret < 2048) {
			perror("read");
			exit(1);
		}

		get_stats(&corrected, &failed);
		printf("byteno: 0x%08x corrected: %d failed: %d\n", i + startbyte, corrected, failed);

		for (j = 0; j < 2048; j++) {
			if (buf[j] != 0xff) {
				printf("swapped: 0x%08x notff: 0x%08x instead: 0x%02x\n",
				       startbyte + i, j, buf[j]);
			}
		}
	}
}

int main(int argc, char *argv[])
{
	int i;
	unsigned int c, f;

	if (argc < 3) {
		printf("usage: %s <mtd> <byte>\n", argv[0]);
		exit (1);
	}

	byte = strtoul(argv[2], NULL, 0);

	fd = open(argv[1], O_RDWR);
	if (fd < 0) {
		perror("open");
		exit(1);
	}

	get_stats(&c, &f);

	buf = malloc(2112);

	for (i = 0; i < 0x840; i+= 64)
		wr_pages(i);

	exit(0);
}
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 02/10] mtd: nand: gpmi: Utilize hardware to detect bitflips in erased blocks

[Boris Brezillon]

On Fri, 8 Dec 2017 11:21:57 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Wed, Dec 06, 2017 at 04:34:31PM +0100, Boris Brezillon wrote:
> > On Wed, 6 Dec 2017 16:28:04 +0100
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > On Wed, Dec 06, 2017 at 10:27:13AM +0100, Boris Brezillon wrote:  
> > > > Hi Sascha,
> > > > 
> > > > On Wed,  6 Dec 2017 10:19:17 +0100
> > > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > >     
> > > > > The GPMI nand has a hardware feature to ignore bitflips in erased pages.
> > > > > Use this feature rather than the longish code we have now.
> > > > > Unfortunately the bitflips in erased pages are not corrected, so we have
> > > > > to memset the read data before passing it to the upper layers.    
> > > > 
> > > > There's a good reason we didn't use the HW bitflip detection in the
> > > > first place: we currently have no way to report the number of corrected
> > > > bitflips in an erased page, and that's a big problem, because then UBI
> > > > does not know when it should re-erase the block.    
> > > 
> > > Ah, ok.
> > >   
> > > > 
> > > > Maybe we missed something when the initial proposal was done and
> > > > there's actually a way to retrieve this information, but if that's not
> > > > the case, I'd prefer to keep the existing implementation even if it's
> > > > slower and more verbose.    
> > > 
> > > I'm not so much concerned about the verbosity of the code but rather
> > > about the magic it has inside. I have stared at this code for some time
> > > now and still only vaguely know what it does.
> > > 
> > > We could do a bit better: We can still detect the number of bitflips
> > > using nand_check_erased_ecc_chunk() without reading the oob data.
> > > That would not be 100% accurate since we do not take the oob data into
> > > account which might have bitflips aswell, but still should be good
> > > enough, no?  
> > 
> > Nope, we really have to take OOB bytes into account when counting
> > bitflips. Say that you have almost all in-band data set to 0xff, but
> > all OOB bytes reserved for ECC are set to 0 because someone decided to
> > write this portion of the page in raw mode. In this case we can't
> > consider the page as empty, otherwise we'll fail to correctly write ECC
> > bytes next time we program the page.  
> 
> I would probably be with you if at least the current code worked correctly,
> but unfortunately it doesn't.
> 
> I did a test with the attached program. What it does is that it writes
> pages nearly full of 0xff in raw mode. In the first page the first byte
> is exchanged with a bitflip, in the second page the second byte is
> exchanged with a bitflip, and so on. What I would expect that the number
> of corrected bits is the same for all bitflip positions. What I get
> instead is this:
> 
> ./a.out /dev/mtd4 0x0f
> byteno: 0x00000000 corrected: 5 failed: 0
> byteno: 0x00000001 corrected: 4 failed: 0
> byteno: 0x00000002 corrected: 5 failed: 0
> byteno: 0x00000003 corrected: 6 failed: 0
> byteno: 0x00000004 corrected: 5 failed: 0
> byteno: 0x00000005 corrected: 5 failed: 0
> byteno: 0x00000006 corrected: 4 failed: 0
> byteno: 0x00000007 corrected: 5 failed: 0
> byteno: 0x00000008 corrected: 5 failed: 0
> byteno: 0x00000009 corrected: 4 failed: 0
> ...
> 
> (Read this as: byteno <x> in page has 0x0f instead of 0xff, so number
> of corrected bits should be 4, instead we have 5)
> 
> or:
> ./a.out /dev/mtd4 0xfe
> byteno: 0x00000000 corrected: 1 failed: 0
> byteno: 0x00000001 corrected: 1 failed: 0
> byteno: 0x00000002 corrected: 1 failed: 0
> byteno: 0x00000003 corrected: 1 failed: 0
> byteno: 0x00000004 corrected: 1 failed: 0
> byteno: 0x00000005 corrected: 1 failed: 0
> byteno: 0x00000006 corrected: 1 failed: 0
> byteno: 0x00000007 corrected: 1 failed: 0
> byteno: 0x00000008 corrected: 1 failed: 0
> byteno: 0x00000009 corrected: 1 failed: 0
> byteno: 0x0000000a corrected: 1 failed: 0
> byteno: 0x0000000b corrected: 1 failed: 0
> byteno: 0x0000000c corrected: 1 failed: 0
> byteno: 0x0000000d corrected: 1 failed: 0
> byteno: 0x0000000e corrected: 2 failed: 0
> byteno: 0x0000000f corrected: 1 failed: 0
> byteno: 0x00000010 corrected: 1 failed: 0
> byteno: 0x00000011 corrected: 1 failed: 0
> byteno: 0x00000012 corrected: 2 failed: 0
> byteno: 0x00000013 corrected: 1 failed: 0
> 
> When I add a print_hex_dump to the driver I really see a buffer
> with 5 bytes flipped instead of 4. When I do a raw read of the
> page (under barebox, the Linux driver doesn't print the OOB data
> properly), then I really see what I have written: A page with four
> bitflips. The results are perfectly reproducable, so I don't expect
> that to be any random read failures.

Wow, that's really weird.

> 
> I know next to nothing about BCH, but could it be that the BCH engine
> already starts correcting the page while it still doesn't know that
> it can't be corrected at all leaving spurious cleared bits in the
> read data?

Bitflips are not supposed to be fixed by the BCH engine if an
uncorrectable error is detected. BCH is operating on a block of data
(usually 512 or 1024 bytes), and before starting to fix actual errors,
it searches their positions, and their positions is only determined
after the correctable/uncorrectable check has been done, so really, I
doubt BCH can mess up you data.

Did you find out where the extra bitflip is? Is it next to the other
bitflips or in a totally different region?

> 
> Sascha
> 
> -------------------------------8<-------------------------------
> 
> #include <stdio.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <mtd/mtd-abi.h>
> #include <sys/ioctl.h>
> #include <stdint.h>
> #include <string.h>
> 
> int fd;
> uint8_t *buf;
> uint8_t byte;
> 
> void erase_block(void)
> {
> 	struct erase_info_user req = {
> 		.start = 0,
> 		.length = 128 * 1024,
> 	};
> 	int ret;
> 
> 	ret = ioctl(fd, MEMERASE, &req);
> 	if (ret != 0) {
> 		perror("ioctl");
> 		exit (1);
> 	}
> }
> 
> void get_stats(unsigned int *__corrected, unsigned int *__failed)
> {
> 	struct mtd_ecc_stats stats;
> 	static unsigned int corrected, failed;
> 	int ret;
> 
> 	ret = ioctl(fd, ECCGETSTATS, &stats);
> 	if (ret != 0) {
> 		perror("ioctl");
> 		exit (1);
>  	}
> 
> 	*__corrected = stats.corrected - corrected;
> 	*__failed = stats.failed - failed;
> 
> 	corrected = stats.corrected;
> 	failed = stats.failed;
> }
> 
> void wr_pages(int startbyte)
> {
> 	struct mtd_write_req req = {
> 		.start = 0,
> 		.len = 2048,
> 		.ooblen = 64,
> 		.usr_data = (__u64)(unsigned long)buf,
> 		.usr_oob = (__u64)(unsigned long)(buf + 2048),
> 		.mode = MTD_OPS_RAW,
> 	};
> 	unsigned int corrected, failed;
> 	int i, ret;
> 
> 	erase_block();
> 
> 	for (i = 0; i < 64; i++) {
> 		memset(buf, 0xff, 2112);
> 
> 		buf[i + startbyte] = byte;
> 
> 		req.start = i * 2048;
> 
> 		ret = ioctl(fd, MEMWRITE, &req);
> 		if (ret != 0) {
> 			perror("ioctl");
> 			exit(1);
> 		}
> 	}
> 
> 	lseek(fd, 0, SEEK_SET);
> 
> 	for (i = 0; i < 64; i++) {
> 		int j;
> 
> 		ret = read(fd, buf, 2048);
> 		if (ret < 2048) {
> 			perror("read");
> 			exit(1);
> 		}
> 
> 		get_stats(&corrected, &failed);
> 		printf("byteno: 0x%08x corrected: %d failed: %d\n", i + startbyte, corrected, failed);
> 
> 		for (j = 0; j < 2048; j++) {
> 			if (buf[j] != 0xff) {
> 				printf("swapped: 0x%08x notff: 0x%08x instead: 0x%02x\n",
> 				       startbyte + i, j, buf[j]);
> 			}
> 		}
> 	}
> }
> 
> int main(int argc, char *argv[])
> {
> 	int i;
> 	unsigned int c, f;
> 
> 	if (argc < 3) {
> 		printf("usage: %s <mtd> <byte>\n", argv[0]);
> 		exit (1);
> 	}
> 
> 	byte = strtoul(argv[2], NULL, 0);
> 
> 	fd = open(argv[1], O_RDWR);
> 	if (fd < 0) {
> 		perror("open");
> 		exit(1);
> 	}
> 
> 	get_stats(&c, &f);
> 
> 	buf = malloc(2112);
> 
> 	for (i = 0; i < 0x840; i+= 64)
> 		wr_pages(i);
> 
> 	exit(0);
> }

Re: [PATCH 02/10] mtd: nand: gpmi: Utilize hardware to detect bitflips in erased blocks

[Sascha Hauer]

On Fri, Dec 08, 2017 at 11:35:50AM +0100, Boris Brezillon wrote:
> On Fri, 8 Dec 2017 11:21:57 +0100
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Wed, Dec 06, 2017 at 04:34:31PM +0100, Boris Brezillon wrote:
> > > On Wed, 6 Dec 2017 16:28:04 +0100
> > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >   
> > > > On Wed, Dec 06, 2017 at 10:27:13AM +0100, Boris Brezillon wrote:  
> > > > > Hi Sascha,
> > > > > 
> > > > > On Wed,  6 Dec 2017 10:19:17 +0100
> > > > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > > >     
> > > > > > The GPMI nand has a hardware feature to ignore bitflips in erased pages.
> > > > > > Use this feature rather than the longish code we have now.
> > > > > > Unfortunately the bitflips in erased pages are not corrected, so we have
> > > > > > to memset the read data before passing it to the upper layers.    
> > > > > 
> > > > > There's a good reason we didn't use the HW bitflip detection in the
> > > > > first place: we currently have no way to report the number of corrected
> > > > > bitflips in an erased page, and that's a big problem, because then UBI
> > > > > does not know when it should re-erase the block.    
> > > > 
> > > > Ah, ok.
> > > >   
> > > > > 
> > > > > Maybe we missed something when the initial proposal was done and
> > > > > there's actually a way to retrieve this information, but if that's not
> > > > > the case, I'd prefer to keep the existing implementation even if it's
> > > > > slower and more verbose.    
> > > > 
> > > > I'm not so much concerned about the verbosity of the code but rather
> > > > about the magic it has inside. I have stared at this code for some time
> > > > now and still only vaguely know what it does.
> > > > 
> > > > We could do a bit better: We can still detect the number of bitflips
> > > > using nand_check_erased_ecc_chunk() without reading the oob data.
> > > > That would not be 100% accurate since we do not take the oob data into
> > > > account which might have bitflips aswell, but still should be good
> > > > enough, no?  
> > > 
> > > Nope, we really have to take OOB bytes into account when counting
> > > bitflips. Say that you have almost all in-band data set to 0xff, but
> > > all OOB bytes reserved for ECC are set to 0 because someone decided to
> > > write this portion of the page in raw mode. In this case we can't
> > > consider the page as empty, otherwise we'll fail to correctly write ECC
> > > bytes next time we program the page.  
> > 
> > I would probably be with you if at least the current code worked correctly,
> > but unfortunately it doesn't.
> > 
> > I did a test with the attached program. What it does is that it writes
> > pages nearly full of 0xff in raw mode. In the first page the first byte
> > is exchanged with a bitflip, in the second page the second byte is
> > exchanged with a bitflip, and so on. What I would expect that the number
> > of corrected bits is the same for all bitflip positions. What I get
> > instead is this:
> > 
> > ./a.out /dev/mtd4 0x0f
> > byteno: 0x00000000 corrected: 5 failed: 0
> > byteno: 0x00000001 corrected: 4 failed: 0
> > byteno: 0x00000002 corrected: 5 failed: 0
> > byteno: 0x00000003 corrected: 6 failed: 0
> > byteno: 0x00000004 corrected: 5 failed: 0
> > byteno: 0x00000005 corrected: 5 failed: 0
> > byteno: 0x00000006 corrected: 4 failed: 0
> > byteno: 0x00000007 corrected: 5 failed: 0
> > byteno: 0x00000008 corrected: 5 failed: 0
> > byteno: 0x00000009 corrected: 4 failed: 0
> > ...
> > 
> > (Read this as: byteno <x> in page has 0x0f instead of 0xff, so number
> > of corrected bits should be 4, instead we have 5)
> > 
> > or:
> > ./a.out /dev/mtd4 0xfe
> > byteno: 0x00000000 corrected: 1 failed: 0
> > byteno: 0x00000001 corrected: 1 failed: 0
> > byteno: 0x00000002 corrected: 1 failed: 0
> > byteno: 0x00000003 corrected: 1 failed: 0
> > byteno: 0x00000004 corrected: 1 failed: 0
> > byteno: 0x00000005 corrected: 1 failed: 0
> > byteno: 0x00000006 corrected: 1 failed: 0
> > byteno: 0x00000007 corrected: 1 failed: 0
> > byteno: 0x00000008 corrected: 1 failed: 0
> > byteno: 0x00000009 corrected: 1 failed: 0
> > byteno: 0x0000000a corrected: 1 failed: 0
> > byteno: 0x0000000b corrected: 1 failed: 0
> > byteno: 0x0000000c corrected: 1 failed: 0
> > byteno: 0x0000000d corrected: 1 failed: 0
> > byteno: 0x0000000e corrected: 2 failed: 0
> > byteno: 0x0000000f corrected: 1 failed: 0
> > byteno: 0x00000010 corrected: 1 failed: 0
> > byteno: 0x00000011 corrected: 1 failed: 0
> > byteno: 0x00000012 corrected: 2 failed: 0
> > byteno: 0x00000013 corrected: 1 failed: 0
> > 
> > When I add a print_hex_dump to the driver I really see a buffer
> > with 5 bytes flipped instead of 4. When I do a raw read of the
> > page (under barebox, the Linux driver doesn't print the OOB data
> > properly), then I really see what I have written: A page with four
> > bitflips. The results are perfectly reproducable, so I don't expect
> > that to be any random read failures.
> 
> Wow, that's really weird.
> 
> > 
> > I know next to nothing about BCH, but could it be that the BCH engine
> > already starts correcting the page while it still doesn't know that
> > it can't be corrected at all leaving spurious cleared bits in the
> > read data?
> 
> Bitflips are not supposed to be fixed by the BCH engine if an
> uncorrectable error is detected. BCH is operating on a block of data
> (usually 512 or 1024 bytes), and before starting to fix actual errors,
> it searches their positions, and their positions is only determined
> after the correctable/uncorrectable check has been done, so really, I
> doubt BCH can mess up you data.
> 
> Did you find out where the extra bitflip is? Is it next to the other
> bitflips or in a totally different region?

It's reproducably the same place, but at arbitrary positions:

See below for a one chunk. The '0xf0' is the bitflip I introduced, no idea
where the '0xbf' and '0xf7' come from:

[   44.223908] data: 00000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.231436] data: 00000010: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.238414] data: 00000020: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.246150] data: 00000030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.253560] data: 00000040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.261099] data: 00000050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.268076] data: 00000060: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.275795] data: 00000070: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.283177] data: 00000080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.290635] data: 00000090: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.297610] data: 000000a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.305274] data: 000000b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.312670] data: 000000c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.319648] data: 000000d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.327403] data: 000000e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.334850] data: 000000f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.342221] data: 00000100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.349197] data: 00000110: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.356878] data: 00000120: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.364258] data: 00000130: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.371704] data: 00000140: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.378678] data: 00000150: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.386335] data: 00000160: ff ff ff ff ff ff ff bf ff ff ff ff ff ff ff ff
[   44.393767] data: 00000170: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.401211] data: 00000180: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.408187] data: 00000190: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f7
[   44.415890] data: 000001a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.423286] data: 000001b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.430716] data: 000001c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.437693] data: 000001d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.445382] data: 000001e0: ff ff ff ff ff ff ff ff ff ff ff f0 ff ff ff ff
[   44.452814] data: 000001f0: ff ff ff ff ff ff ff ff ff ff ff ff f7 ff ff ff
[   44.459792] ecc: 00000000: ff ff ff ff ff ff ff ff ff ff ff ff ff

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |