[PATCH 0/3] mtd: rawnand: marvell: add new layout

[PATCH 0/3] mtd: rawnand: marvell: add new layout

[Miquel Raynal]

Hello,

This series is an attempt at supporting a new layout for Marvell NAND
controller: NAND chips using 2kiB pages, requesting at least 8
correctable bits per 512-byte chunks.

While this could have been trivial by the simple addition of this
layout in the marvell_nfc_layout table as described in Marvell (not
public) AN-379, actual testing shown a strange error. With this
layout, one bitflip appears on the second chunk of every erased page
when we read with BCH ECC engine activated.

It has been experimentally checked that using 64 free OOB bytes
instead of 32 in the second chunk cancels this bitflip. However, this
is not a viable solution as the BootROM would not find the ECC bytes
at the right place and would declare bad any block written with this
layout.

The solution found was to re-read in raw mode the whole page instead
of just the ECC bytes when an ECC error occurs to check if the entire
page is empty or not.

Also, with this layout, Bad Block Markers (BBM) are expected to be
within the data area. Prevent any use of the BBM in this case.

The first patch reworks the ECC failure path of the BCH read function,
the second patch adds support for the new layout with the above
problems addressed and the last patch is just an optimization of the
same ECC failure path for the Hamming read function.

Thanks,
Miquèl


Miquel Raynal (3):
  mtd: rawnand: marvell: rework BCH engine failure path
  mtd: rawnand: marvell: support 8b/512B strength for 2kiB pages layout
  mtd: rawnand: marvell: speed-up hamming failure path

 drivers/mtd/nand/raw/marvell_nand.c | 123 +++++++++++++++++++++++-------------
 1 file changed, 79 insertions(+), 44 deletions(-)

-- 
2.14.1

[PATCH 1/3] mtd: rawnand: marvell: rework BCH engine failure path

[Miquel Raynal]

We are about to support a new layout that triggers a faulty mechanism in
BCH engine that creates bitflips in erased pages.

Before adding the quirk that will workaround this issue, this patch just
reworks a bit the section that handles ECC failures in BCH read path.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/marvell_nand.c | 75 +++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 29 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 80a074cccb82..954b5755ba59 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -1234,11 +1234,11 @@ static int marvell_nfc_hw_ecc_bch_read_page(struct mtd_info *mtd,
 					    int page)
 {
 	const struct marvell_hw_ecc_layout *lt = to_marvell_nand(chip)->layout;
-	int data_len = lt->data_bytes, spare_len = lt->spare_bytes, ecc_len;
-	u8 *data = buf, *spare = chip->oob_poi, *ecc;
+	int data_len = lt->data_bytes, spare_len = lt->spare_bytes;
+	u8 *data = buf, *spare = chip->oob_poi;
 	int max_bitflips = 0;
 	u32 failure_mask = 0;
-	int chunk, ecc_offset_in_page, ret;
+	int chunk, ret;
 
 	/*
 	 * With BCH, OOB is not fully used (and thus not read entirely), not
@@ -1279,46 +1279,63 @@ static int marvell_nfc_hw_ecc_bch_read_page(struct mtd_info *mtd,
 	 * the controller in normal mode and must be re-read in raw mode. To
 	 * avoid dropping the performances, we prefer not to include them. The
 	 * user should re-read the page in raw mode if ECC bytes are required.
+	 */
+
+	/*
+	 * In case there is any subpage read error reported by ->correct(), we
+	 * usually re-read only ECC bytes in raw mode and check if the whole
+	 * page is empty. In this case, it is normal that the ECC check failed
+	 * and we just ignore the error.
 	 *
 	 * However, for any subpage read error reported by ->correct(), the ECC
 	 * bytes must be read in raw mode and the full subpage must be checked
 	 * to see if it is entirely empty of if there was an actual error.
 	 */
 	for (chunk = 0; chunk < lt->nchunks; chunk++) {
+		int data_off_in_page, spare_off_in_page, ecc_off_in_page;
+		int data_off, spare_off, ecc_off;
+		int data_len, spare_len, ecc_len;
+
 		/* No failure reported for this chunk, move to the next one */
 		if (!(failure_mask & BIT(chunk)))
 			continue;
 
-		/* Derive ECC bytes positions (in page/buffer) and length */
-		ecc = chip->oob_poi +
-			(lt->full_chunk_cnt * lt->spare_bytes) +
-			lt->last_spare_bytes +
-			(chunk * ALIGN(lt->ecc_bytes, 32));
-		ecc_offset_in_page =
-			(chunk * (lt->data_bytes + lt->spare_bytes +
-				  lt->ecc_bytes)) +
-			(chunk < lt->full_chunk_cnt ?
-			 lt->data_bytes + lt->spare_bytes :
-			 lt->last_data_bytes + lt->last_spare_bytes);
-		ecc_len = chunk < lt->full_chunk_cnt ?
-			lt->ecc_bytes : lt->last_ecc_bytes;
+		/*
+		 * Only re-read the ECC bytes, unless we are using the 2k/8b
+		 * layout which is buggy in the sense that the ECC engine will
+		 * try to correct data bytes anyway, creating bitflips. In this
+		 * case, re-read the entire page.
+		 */
+		data_off_in_page = chunk * (lt->data_bytes + lt->spare_bytes +
+					    lt->ecc_bytes);
+		spare_off_in_page = data_off_in_page +
+			(chunk < lt->full_chunk_cnt ? lt->data_bytes :
+						      lt->last_data_bytes);
+		ecc_off_in_page = spare_off_in_page +
+			(chunk < lt->full_chunk_cnt ? lt->spare_bytes :
+						      lt->last_spare_bytes);
 
-		/* Do the actual raw read of the ECC bytes */
-		nand_change_read_column_op(chip, ecc_offset_in_page,
-					   ecc, ecc_len, false);
+		data_off = chunk * lt->data_bytes;
+		spare_off = chunk * lt->spare_bytes;
+		ecc_off = (lt->full_chunk_cnt * lt->spare_bytes) +
+			  lt->last_spare_bytes +
+			  (chunk * (lt->ecc_bytes + 2));
 
-		/* Derive data/spare bytes positions (in buffer) and length */
-		data = buf + (chunk * lt->data_bytes);
-		data_len = chunk < lt->full_chunk_cnt ?
-			lt->data_bytes : lt->last_data_bytes;
-		spare = chip->oob_poi + (chunk * (lt->spare_bytes +
-						  lt->ecc_bytes));
-		spare_len = chunk < lt->full_chunk_cnt ?
-			lt->spare_bytes : lt->last_spare_bytes;
+		data_len = chunk < lt->full_chunk_cnt ? lt->data_bytes :
+							lt->last_data_bytes;
+		spare_len = chunk < lt->full_chunk_cnt ? lt->spare_bytes :
+							 lt->last_spare_bytes;
+		ecc_len = chunk < lt->full_chunk_cnt ? lt->ecc_bytes :
+						       lt->last_ecc_bytes;
+
+		nand_change_read_column_op(chip, ecc_off_in_page,
+					   chip->oob_poi + ecc_off, ecc_len,
+					   false);
 
 		/* Check the entire chunk (data + spare + ecc) for emptyness */
-		marvell_nfc_check_empty_chunk(chip, data, data_len, spare,
-					      spare_len, ecc, ecc_len,
+		marvell_nfc_check_empty_chunk(chip, buf + data_off, data_len,
+					      chip->oob_poi + spare_off, spare_len,
+					      chip->oob_poi + ecc_off, ecc_len,
 					      &max_bitflips);
 	}
 
-- 
2.14.1

[PATCH 2/3] mtd: rawnand: marvell: support 8b/512B strength for 2kiB pages layout

[Miquel Raynal]

Add support for the layout used by 2kiB page NAND chips requesting at
least 8-bit of correction per 512 bytes. This layout requires a bit of
handling as:
1/ It can only fit if the NAND chip has at least 128 OOB bytes.
2/ The Bad Block Markers are located in the middle of the data bytes
   and shall not be used.
3/ It has been experimentally observed that, for certain layouts, the ECC
   engine tries to correct data while it should not because the errors
   are uncorrectable. While this is harmless for truly bad pages, it
   creates bitflips in empty pages. To avoid such scenario that
   augments artificially the number of bitflips we re-read in raw mode
   the entire page instead of just the ECC bytes. This is done only
   for this layout to avoid an unneeded penalty with other setups.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/marvell_nand.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 954b5755ba59..83bb4ef9baa6 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -217,6 +217,7 @@ static const struct marvell_hw_ecc_layout marvell_nfc_layouts[] = {
 	MARVELL_LAYOUT(  512,   512,  1,  1,  1,  512,  8,  8,  0,  0,  0),
 	MARVELL_LAYOUT( 2048,   512,  1,  1,  1, 2048, 40, 24,  0,  0,  0),
 	MARVELL_LAYOUT( 2048,   512,  4,  1,  1, 2048, 32, 30,  0,  0,  0),
+	MARVELL_LAYOUT( 2048,   512,  8,  2,  1, 1024,  0, 30,1024,32, 30),
 	MARVELL_LAYOUT( 4096,   512,  4,  2,  2, 2048, 32, 30,  0,  0,  0),
 	MARVELL_LAYOUT( 4096,   512,  8,  5,  4, 1024,  0, 30,  0, 64, 30),
 };
@@ -1287,9 +1288,11 @@ static int marvell_nfc_hw_ecc_bch_read_page(struct mtd_info *mtd,
 	 * page is empty. In this case, it is normal that the ECC check failed
 	 * and we just ignore the error.
 	 *
-	 * However, for any subpage read error reported by ->correct(), the ECC
-	 * bytes must be read in raw mode and the full subpage must be checked
-	 * to see if it is entirely empty of if there was an actual error.
+	 * However, it has been empirically observed that for some layouts (e.g
+	 * 2k page, 8b strength per 512B chunk), the controller tries to correct
+	 * bits and may create itself bitflips in the erased area. To overcome
+	 * this strange behavior, the whole page is re-read in raw mode, not
+	 * only the ECC bytes.
 	 */
 	for (chunk = 0; chunk < lt->nchunks; chunk++) {
 		int data_off_in_page, spare_off_in_page, ecc_off_in_page;
@@ -1328,6 +1331,15 @@ static int marvell_nfc_hw_ecc_bch_read_page(struct mtd_info *mtd,
 		ecc_len = chunk < lt->full_chunk_cnt ? lt->ecc_bytes :
 						       lt->last_ecc_bytes;
 
+		if (lt->writesize == 2048 && lt->strength == 8) {
+			nand_change_read_column_op(chip, data_off_in_page,
+						   buf + data_off, data_len,
+						   false);
+			nand_change_read_column_op(chip, spare_off_in_page,
+						   chip->oob_poi + spare_off, spare_len,
+						   false);
+		}
+
 		nand_change_read_column_op(chip, ecc_off_in_page,
 					   chip->oob_poi + ecc_off, ecc_len,
 					   false);
@@ -2112,6 +2124,16 @@ static int marvell_nand_hw_ecc_ctrl_init(struct mtd_info *mtd,
 		return -ENOTSUPP;
 	}
 
+	/* Special care for the layout 2k/8-bit/512B  */
+	if (l->writesize == 2048 && l->strength == 8) {
+		if (mtd->oobsize < 128) {
+			dev_err(nfc->dev, "Requested layout needs at least 128 OOB bytes\n");
+			return -ENOTSUPP;
+		} else {
+			chip->bbt_options |= NAND_BBT_NO_OOB_BBM;
+		}
+	}
+
 	mtd_set_ooblayout(mtd, &marvell_nand_ooblayout_ops);
 	ecc->steps = l->nchunks;
 	ecc->size = l->data_bytes;
-- 
2.14.1

[PATCH 3/3] mtd: rawnand: marvell: speed-up hamming failure path

[Miquel Raynal]

When a read triggers an ECC error it might have two causes:
* Either there was too much bitflips and the data cannot be sanitized.
* Or the page is empty.

For the latter case, we need to re-read the page in raw mode to check
for the ECC bytes to be 0xFF. This is done with a call to
marvell_nfc_hw_ecc_hmg_do_read_page() which will trigger a new read
inside the NAND chip and implies strong delays. As the data is already
laying in the NAND cache, let's just retrieve it with the ECC engine
turned off.

As we are only reading a small section of the page, we can write
directly in the OOB buffer at the right offset, there is no need to
allocate a buffer for that.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/marvell_nand.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 83bb4ef9baa6..29cb3d2e121f 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -974,9 +974,7 @@ static int marvell_nfc_hw_ecc_hmg_read_page(struct mtd_info *mtd,
 					    int page)
 {
 	const struct marvell_hw_ecc_layout *lt = to_marvell_nand(chip)->layout;
-	unsigned int full_sz = lt->data_bytes + lt->spare_bytes + lt->ecc_bytes;
 	int max_bitflips = 0, ret;
-	u8 *raw_buf;
 
 	marvell_nfc_enable_hw_ecc(chip);
 	marvell_nfc_hw_ecc_hmg_do_read_page(chip, buf, chip->oob_poi, false,
@@ -988,18 +986,16 @@ static int marvell_nfc_hw_ecc_hmg_read_page(struct mtd_info *mtd,
 		return max_bitflips;
 
 	/*
-	 * When ECC failures are detected, check if the full page has been
-	 * written or not. Ignore the failure if it is actually empty.
+	 * When ECC failures are detected, check if the page has been
+	 * written or not by re-reading the ECC bytes in raw mode.
+	 * Ignore the failure if it is actually empty.
 	 */
-	raw_buf = kmalloc(full_sz, GFP_KERNEL);
-	if (!raw_buf)
-		return -ENOMEM;
-
-	marvell_nfc_hw_ecc_hmg_do_read_page(chip, raw_buf, raw_buf +
-					    lt->data_bytes, true, page);
-	marvell_nfc_check_empty_chunk(chip, raw_buf, full_sz, NULL, 0, NULL, 0,
+	nand_change_read_column_op(chip, lt->data_bytes + lt->spare_bytes,
+				   chip->oob_poi + lt->spare_bytes,
+				   lt->ecc_bytes, false);
+	marvell_nfc_check_empty_chunk(chip, buf, lt->data_bytes, chip->oob_poi,
+				      lt->spare_bytes + lt->ecc_bytes, NULL, 0,
 				      &max_bitflips);
-	kfree(raw_buf);
 
 	return max_bitflips;
 }
-- 
2.14.1

Re: [PATCH 0/3] mtd: rawnand: marvell: add new layout

[Boris Brezillon]

On Thu, 19 Jul 2018 17:23:34 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello,
> 
> This series is an attempt at supporting a new layout for Marvell NAND
> controller: NAND chips using 2kiB pages, requesting at least 8
> correctable bits per 512-byte chunks.
> 
> While this could have been trivial by the simple addition of this
> layout in the marvell_nfc_layout table as described in Marvell (not
> public) AN-379, actual testing shown a strange error. With this
> layout, one bitflip appears on the second chunk of every erased page
> when we read with BCH ECC engine activated.
> 
> It has been experimentally checked that using 64 free OOB bytes
> instead of 32 in the second chunk cancels this bitflip. However, this
> is not a viable solution as the BootROM would not find the ECC bytes
> at the right place and would declare bad any block written with this
> layout.
> 
> The solution found was to re-read in raw mode the whole page instead
> of just the ECC bytes when an ECC error occurs to check if the entire
> page is empty or not.
> 
> Also, with this layout, Bad Block Markers (BBM) are expected to be
> within the data area. Prevent any use of the BBM in this case.
> 
> The first patch reworks the ECC failure path of the BCH read function,
> the second patch adds support for the new layout with the above
> problems addressed and the last patch is just an optimization of the
> same ECC failure path for the Hamming read function.
> 
> Thanks,
> Miquèl
> 
> 
> Miquel Raynal (3):
>   mtd: rawnand: marvell: rework BCH engine failure path
>   mtd: rawnand: marvell: support 8b/512B strength for 2kiB pages layout
>   mtd: rawnand: marvell: speed-up hamming failure path

The whole series is

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> 
>  drivers/mtd/nand/raw/marvell_nand.c | 123 +++++++++++++++++++++++-------------
>  1 file changed, 79 insertions(+), 44 deletions(-)
> 

Re: [PATCH 0/3] mtd: rawnand: marvell: add new layout

[Miquel Raynal]

Hello,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Thu, 19 Jul 2018
17:23:34 +0200:

> Hello,
> 
> This series is an attempt at supporting a new layout for Marvell NAND
> controller: NAND chips using 2kiB pages, requesting at least 8
> correctable bits per 512-byte chunks.
> 
> While this could have been trivial by the simple addition of this
> layout in the marvell_nfc_layout table as described in Marvell (not
> public) AN-379, actual testing shown a strange error. With this
> layout, one bitflip appears on the second chunk of every erased page
> when we read with BCH ECC engine activated.
> 
> It has been experimentally checked that using 64 free OOB bytes
> instead of 32 in the second chunk cancels this bitflip. However, this
> is not a viable solution as the BootROM would not find the ECC bytes
> at the right place and would declare bad any block written with this
> layout.
> 
> The solution found was to re-read in raw mode the whole page instead
> of just the ECC bytes when an ECC error occurs to check if the entire
> page is empty or not.
> 
> Also, with this layout, Bad Block Markers (BBM) are expected to be
> within the data area. Prevent any use of the BBM in this case.
> 
> The first patch reworks the ECC failure path of the BCH read function,
> the second patch adds support for the new layout with the above
> problems addressed and the last patch is just an optimization of the
> same ECC failure path for the Hamming read function.
> 
> Thanks,
> Miquèl
> 
> 
> Miquel Raynal (3):
>   mtd: rawnand: marvell: rework BCH engine failure path
>   mtd: rawnand: marvell: support 8b/512B strength for 2kiB pages layout
>   mtd: rawnand: marvell: speed-up hamming failure path
> 
>  drivers/mtd/nand/raw/marvell_nand.c | 123 +++++++++++++++++++++++-------------
>  1 file changed, 79 insertions(+), 44 deletions(-)
> 

Patches 1 and 2 applied, patch 3 dropped because it would have cause
pxa (NFCv1) not to work anymore because of the unsupported "change
column".

Thanks,
Miquèl