[PATCH v2 00/14] Update for QCOM NAND driver

[PATCH v2 00/14] Update for QCOM NAND driver

[Abhishek Sahu]

* v2:

1. Addressed all review comments in v1.
1. Make the generic helper function for NAND ECC parameters setup
   and used this helper function for QCOM and Denali nand driver
   for ECC setup.
2. Modified commit message for some of the patches and added more
   comments.
3. Added new patch for fixing ‘return 0’ for raw read.
4. Removed the read last codeword part for nand oob write.
5. Reorganized bad block check function and removed the
   read_last_cw function completely.

* v1:

This patch series mainly deals with error handling and erased page
bitflip detection for QCOM NAND driver.

1. The error handling was missing for some of the cases so fixed
   the same.

2. Add the support for taking ECC strength from ONFI parameter.
   The earlier QCOM boards were coming with 4-bit ECC chip but
   now the same boards are coming with 8-bit ECC chip since the
   earlier 4-bit parts are obsolete from some vendors.

3. We got few issues related with NAND erased page bitflips. The
   QCOM NAND controller can’t detect the bitflip in completely erased
   page so added the support to detect the same. It implemented the
   logic mentioned in patch [1] which didn’t go in mainline and later
   the generic functions were provided [2] to count the number of
   bitflips and make all 0xff. This patch series did some optimization
   logic to prevent the unnecessary full page raw read and data copy
   from QCOM NAND controller to DMA.

4. Following are the testing done for these patches in QCOM IPQ8074
   HK01 (4-bit and 8-bit ECC chip) and IPQ806x AP148 boards.
    a. Run all mtd test and check if it passes
    b. Introduce custom bitflips in erased page and check if it
       returns no error/EUCLEAN/EBADMSG depending upon number of
       bitflips and position.
    c. Introduce failure condition for operational failure and
       check if it detects the same.

[1]: https://patchwork.ozlabs.org/patch/328994/
[2]: https://patchwork.ozlabs.org/patch/509970/

Abhishek Sahu (14):
  mtd: rawnand: helper function for setting up ECC parameters
  mtd: rawnand: denali: use helper function for ecc setup
  dt-bindings: qcom_nandc: make nand-ecc-strength optional
  mtd: rawnand: qcom: use the ecc strength from device parameter
  mtd: rawnand: qcom: wait for desc completion in all BAM channels
  mtd: rawnand: qcom: erased page detection for uncorrectable errors
    only
  mtd: rawnand: qcom: fix null pointer access for erased page detection
  mtd: rawnand: qcom: parse read errors for read oob also
  mtd: rawnand: qcom: modify write_oob to remove read codeword part
  mtd: rawnand: qcom: fix return value for raw page read
  mtd: rawnand: qcom: minor code reorganization for bad block check
  mtd: rawnand: qcom: check for operation errors in case of raw read
  mtd: rawnand: qcom: helper function for raw read
  mtd: rawnand: qcom: erased page bitflips detection

 .../devicetree/bindings/mtd/qcom_nandc.txt         |   4 +-
 drivers/mtd/nand/raw/denali.c                      |  30 +-
 drivers/mtd/nand/raw/nand_base.c                   |  42 ++
 drivers/mtd/nand/raw/qcom_nandc.c                  | 622 ++++++++++++++-------
 include/linux/mtd/rawnand.h                        |   3 +
 5 files changed, 461 insertions(+), 240 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters

[Abhishek Sahu]

commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
match, maximize ECC settings") provides generic helpers which
drivers can use for setting up ECC parameters.

Since same board can have different ECC strength nand chips so
following is the logic for setting up ECC strength and ECC step
size, which can be used by most of the drivers.

1. If both ECC step size and ECC strength are already set
   (usually by DT) then just check whether this setting
   is supported by NAND controller.
2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
   supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest
   to the chip's requirement. If available OOB size can't fit the chip
   requirement then select maximum ECC strength which can be fit with
   available OOB size with warning.

This patch introduces nand_ecc_param_setup function which calls the
required helper functions for the above logic. The drivers can use
this single function instead of calling the 3 helper functions
individually.

CC: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

  NEW PATCH

 drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/rawnand.h      |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..dd7a984 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
 }
 EXPORT_SYMBOL_GPL(nand_maximize_ecc);
 
+/**
+ * nand_ecc_param_setup - Set the ECC strength and ECC step size
+ * @chip: nand chip info structure
+ * @caps: ECC engine caps info structure
+ * @oobavail: OOB size that the ECC engine can use
+ *
+ * Choose the ECC strength according to following logic
+ *
+ * 1. If both ECC step size and ECC strength are already set (usually by DT)
+ *    then check if it is supported by this controller.
+ * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
+ * 3. Otherwise, try to match the ECC step size and ECC strength closest
+ *    to the chip's requirement. If available OOB size can't fit the chip
+ *    requirement then fallback to the maximum ECC step size and ECC strength
+ *    and print the warning.
+ *
+ * On success, the chosen ECC settings are set.
+ */
+int nand_ecc_param_setup(struct nand_chip *chip,
+			 const struct nand_ecc_caps *caps, int oobavail)
+{
+	int ret;
+
+	if (chip->ecc.size && chip->ecc.strength)
+		return nand_check_ecc_caps(chip, caps, oobavail);
+
+	if (chip->ecc.options & NAND_ECC_MAXIMIZE)
+		return nand_maximize_ecc(chip, caps, oobavail);
+
+	if (!nand_match_ecc_req(chip, caps, oobavail))
+		return 0;
+
+	ret = nand_maximize_ecc(chip, caps, oobavail);
+	if (!ret)
+		pr_warn("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n",
+			chip->ecc_step_ds, chip->ecc_strength_ds,
+			chip->ecc.size, chip->ecc.strength);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nand_ecc_param_setup);
+
 /*
  * Check if the chip configuration meet the datasheet requirements.
 
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 5dad59b..afc7447 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1627,6 +1627,9 @@ int nand_match_ecc_req(struct nand_chip *chip,
 int nand_maximize_ecc(struct nand_chip *chip,
 		      const struct nand_ecc_caps *caps, int oobavail);
 
+int nand_ecc_param_setup(struct nand_chip *chip,
+			 const struct nand_ecc_caps *caps, int oobavail);
+
 /* Default write_oob implementation */
 int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 02/14] mtd: rawnand: denali: use helper function for ecc setup

[Abhishek Sahu]

Now, the NAND base layer has helper function for ecc
parameters setup which does the same thing.

CC: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

  NEW PATCH

 drivers/mtd/nand/raw/denali.c | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index 2a302a1..d75f4e5 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -1120,33 +1120,6 @@ int denali_calc_ecc_bytes(int step_size, int strength)
 }
 EXPORT_SYMBOL(denali_calc_ecc_bytes);
 
-static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
-			    struct denali_nand_info *denali)
-{
-	int oobavail = mtd->oobsize - denali->oob_skip_bytes;
-	int ret;
-
-	/*
-	 * If .size and .strength are already set (usually by DT),
-	 * check if they are supported by this controller.
-	 */
-	if (chip->ecc.size && chip->ecc.strength)
-		return nand_check_ecc_caps(chip, denali->ecc_caps, oobavail);
-
-	/*
-	 * We want .size and .strength closest to the chip's requirement
-	 * unless NAND_ECC_MAXIMIZE is requested.
-	 */
-	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE)) {
-		ret = nand_match_ecc_req(chip, denali->ecc_caps, oobavail);
-		if (!ret)
-			return 0;
-	}
-
-	/* Max ECC strength is the last thing we can do */
-	return nand_maximize_ecc(chip, denali->ecc_caps, oobavail);
-}
-
 static int denali_ooblayout_ecc(struct mtd_info *mtd, int section,
 				struct mtd_oob_region *oobregion)
 {
@@ -1317,7 +1290,8 @@ int denali_init(struct denali_nand_info *denali)
 	chip->ecc.mode = NAND_ECC_HW_SYNDROME;
 	chip->options |= NAND_NO_SUBPAGE_WRITE;
 
-	ret = denali_ecc_setup(mtd, chip, denali);
+	ret = nand_ecc_param_setup(chip, denali->ecc_caps,
+				   mtd->oobsize - denali->oob_skip_bytes);
 	if (ret) {
 		dev_err(denali->dev, "Failed to setup ECC settings.\n");
 		goto disable_irq;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional

[Abhishek Sahu]

Now, nand-ecc-strength is optional. If specified in DT, then
controller will use this ECC strength otherwise ECC strength will
be calculated according to chip requirement and available OOB size.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

  NEW PATCH

 Documentation/devicetree/bindings/mtd/qcom_nandc.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
index 73d336be..f246aa0 100644
--- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
+++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
@@ -45,11 +45,13 @@ Required properties:
 			number (e.g., 0, 1, 2, etc.)
 - #address-cells:	see partition.txt
 - #size-cells:		see partition.txt
-- nand-ecc-strength:	see nand.txt
 - nand-ecc-step-size:	must be 512. see nand.txt for more details.
 
 Optional properties:
 - nand-bus-width:	see nand.txt
+- nand-ecc-strength:	see nand.txt. If not specified, then ECC strength will
+			be used according to chip requirement and available
+			OOB size.
 
 Each nandcs device node may optionally contain a 'partitions' sub-node, which
 further contains sub-nodes describing the flash partition mapping. See
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional

[Abhishek Sahu]

Now, nand-ecc-strength is optional. If specified in DT, then
controller will use this ECC strength otherwise ECC strength will
be calculated according to chip requirement and available OOB size.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

  NEW PATCH

 Documentation/devicetree/bindings/mtd/qcom_nandc.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
index 73d336be..f246aa0 100644
--- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
+++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
@@ -45,11 +45,13 @@ Required properties:
 			number (e.g., 0, 1, 2, etc.)
 - #address-cells:	see partition.txt
 - #size-cells:		see partition.txt
-- nand-ecc-strength:	see nand.txt
 - nand-ecc-step-size:	must be 512. see nand.txt for more details.
 
 Optional properties:
 - nand-bus-width:	see nand.txt
+- nand-ecc-strength:	see nand.txt. If not specified, then ECC strength will
+			be used according to chip requirement and available
+			OOB size.
 
 Each nandcs device node may optionally contain a 'partitions' sub-node, which
 further contains sub-nodes describing the flash partition mapping. See
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 04/14] mtd: rawnand: qcom: use the ecc strength from device parameter

[Abhishek Sahu]

Currently the driver uses the ECC strength specified in DT.
The QPIC/EBI2 NAND supports 4 or 8-bit ECC correction. The same
kind of board can have different NAND parts so use the ECC
strength from device parameters if it is not specified in DT.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

  1. Removed the custom logic and used the helper fuction.

 drivers/mtd/nand/raw/qcom_nandc.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index b554fb6..a8d71ce 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2315,13 +2315,21 @@ static int qcom_nand_ooblayout_free(struct mtd_info *mtd, int section,
 	.free = qcom_nand_ooblayout_free,
 };
 
+static int
+qcom_nandc_calc_ecc_bytes(int step_size, int strength)
+{
+	return strength == 4 ? 12 : 16;
+}
+NAND_ECC_CAPS_SINGLE(qcom_nandc_ecc_caps, qcom_nandc_calc_ecc_bytes,
+		     NANDC_STEP_SIZE, 4, 8);
+
 static int qcom_nand_host_setup(struct qcom_nand_host *host)
 {
 	struct nand_chip *chip = &host->chip;
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
-	int cwperpage, bad_block_byte;
+	int cwperpage, bad_block_byte, ret;
 	bool wide_bus;
 	int ecc_mode = 1;
 
@@ -2334,8 +2342,20 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
 		return -EINVAL;
 	}
 
+	cwperpage = mtd->writesize / ecc->size;
+
+	/*
+	 * Each CW has 4 available OOB bytes which will be protected with ECC
+	 * so remaining bytes can be used for ECC.
+	 */
+	ret = nand_ecc_param_setup(chip, &qcom_nandc_ecc_caps,
+				   mtd->oobsize - (cwperpage << 2));
+	if (ret) {
+		dev_err(nandc->dev, "No valid ecc settings possible\n");
+		return ret;
+	}
+
 	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
-
 	if (ecc->strength >= 8) {
 		/* 8 bit ECC defaults to BCH ECC on all platforms */
 		host->bch_enabled = true;
@@ -2403,7 +2423,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
 
 	mtd_set_ooblayout(mtd, &qcom_nand_ooblayout_ops);
 
-	cwperpage = mtd->writesize / ecc->size;
 	nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage,
 				     cwperpage);
 
@@ -2419,12 +2438,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
 	 * for 8 bit ECC
 	 */
 	host->cw_size = host->cw_data + ecc->bytes;
-
-	if (ecc->bytes * (mtd->writesize / ecc->size) > mtd->oobsize) {
-		dev_err(nandc->dev, "ecc data doesn't fit in OOB area\n");
-		return -EINVAL;
-	}
-
 	bad_block_byte = mtd->writesize - host->cw_size * (cwperpage - 1) + 1;
 
 	host->cfg0 = (cwperpage - 1) << CW_PER_PAGE
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 05/14] mtd: rawnand: qcom: wait for desc completion in all BAM channels

[Abhishek Sahu]

The BAM has 3 channels - tx, rx and command. command channel
is used for register read/writes, tx channel for data writes
and rx channel for data reads. Currently, the driver assumes the
transfer completion once it gets all the command descriptor
completed. Sometimes, there is race condition in data channel
(tx/rx) and command channel completion and in these cases,
the data in buffer is not valid during the small window between
command descriptor completion and data descriptor completion.

Now, the changes have been made to assign the callback for
channel's final descriptor. The DMA will generate the callback
when all the descriptors have completed in that channel.
The NAND transfer will be completed only when all required
DMA channels have generated the completion callback.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

  NONE

  1. Removed the custom logic and used the helper fuction.
 drivers/mtd/nand/raw/qcom_nandc.c | 55 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index a8d71ce..3d1ff54 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -213,6 +213,8 @@
 #define QPIC_PER_CW_CMD_SGL		32
 #define QPIC_PER_CW_DATA_SGL		8
 
+#define QPIC_NAND_COMPLETION_TIMEOUT	msecs_to_jiffies(2000)
+
 /*
  * Flags used in DMA descriptor preparation helper functions
  * (i.e. read_reg_dma/write_reg_dma/read_data_dma/write_data_dma)
@@ -245,6 +247,11 @@
  * @tx_sgl_start - start index in data sgl for tx.
  * @rx_sgl_pos - current index in data sgl for rx.
  * @rx_sgl_start - start index in data sgl for rx.
+ * @first_chan_done - if current transfer already has got first channel
+ *		      DMA desc completion.
+ * @txn_done - completion for nand transfer.
+ * @last_data_desc - last DMA desc in data channel (tx/rx).
+ * @last_cmd_desc - last DMA desc in command channel.
  */
 struct bam_transaction {
 	struct bam_cmd_element *bam_ce;
@@ -258,6 +265,10 @@ struct bam_transaction {
 	u32 tx_sgl_start;
 	u32 rx_sgl_pos;
 	u32 rx_sgl_start;
+	bool first_chan_done;
+	struct completion txn_done;
+	struct dma_async_tx_descriptor *last_data_desc;
+	struct dma_async_tx_descriptor *last_cmd_desc;
 };
 
 /*
@@ -504,6 +515,8 @@ static void free_bam_transaction(struct qcom_nand_controller *nandc)
 
 	bam_txn->data_sgl = bam_txn_buf;
 
+	init_completion(&bam_txn->txn_done);
+
 	return bam_txn;
 }
 
@@ -523,11 +536,36 @@ static void clear_bam_transaction(struct qcom_nand_controller *nandc)
 	bam_txn->tx_sgl_start = 0;
 	bam_txn->rx_sgl_pos = 0;
 	bam_txn->rx_sgl_start = 0;
+	bam_txn->last_data_desc = NULL;
+	bam_txn->first_chan_done = false;
 
 	sg_init_table(bam_txn->cmd_sgl, nandc->max_cwperpage *
 		      QPIC_PER_CW_CMD_SGL);
 	sg_init_table(bam_txn->data_sgl, nandc->max_cwperpage *
 		      QPIC_PER_CW_DATA_SGL);
+
+	reinit_completion(&bam_txn->txn_done);
+}
+
+/* Callback for DMA descriptor completion */
+static void qpic_bam_dma_done(void *data)
+{
+	struct bam_transaction *bam_txn = data;
+
+	/*
+	 * In case of data transfer with NAND, 2 callbacks will be generated.
+	 * One for command channel and another one for data channel.
+	 * If current transaction has data descriptors then check if its
+	 * already got one DMA channel completion callback. In this case
+	 * make the NAND transfer complete otherwise mark first_chan_done true
+	 * and wait for next channel DMA completion callback.
+	 */
+	if (bam_txn->last_data_desc && !bam_txn->first_chan_done) {
+		bam_txn->first_chan_done = true;
+		return;
+	}
+
+	complete(&bam_txn->txn_done);
 }
 
 static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip)
@@ -756,6 +794,12 @@ static int prepare_bam_async_desc(struct qcom_nand_controller *nandc,
 
 	desc->dma_desc = dma_desc;
 
+	/* update last data/command descriptor */
+	if (chan == nandc->cmd_chan)
+		bam_txn->last_cmd_desc = dma_desc;
+	else
+		bam_txn->last_data_desc = dma_desc;
+
 	list_add_tail(&desc->node, &nandc->desc_list);
 
 	return 0;
@@ -1273,10 +1317,19 @@ static int submit_descs(struct qcom_nand_controller *nandc)
 		cookie = dmaengine_submit(desc->dma_desc);
 
 	if (nandc->props->is_bam) {
+		bam_txn->last_cmd_desc->callback = qpic_bam_dma_done;
+		bam_txn->last_cmd_desc->callback_param = bam_txn;
+		if (bam_txn->last_data_desc) {
+			bam_txn->last_data_desc->callback = qpic_bam_dma_done;
+			bam_txn->last_data_desc->callback_param = bam_txn;
+		}
+
 		dma_async_issue_pending(nandc->tx_chan);
 		dma_async_issue_pending(nandc->rx_chan);
+		dma_async_issue_pending(nandc->cmd_chan);
 
-		if (dma_sync_wait(nandc->cmd_chan, cookie) != DMA_COMPLETE)
+		if (!wait_for_completion_timeout(&bam_txn->txn_done,
+						 QPIC_NAND_COMPLETION_TIMEOUT))
 			return -ETIMEDOUT;
 	} else {
 		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 06/14] mtd: rawnand: qcom: erased page detection for uncorrectable errors only

[Abhishek Sahu]

Following is the flow in the HW if controller tries to read erased
page

1. First ECC uncorrectable error will be generated from ECC engine
   since ECC engine first calculates the ECC with all 0xff and match
   the calculated ECC with ECC code in OOB (which is again all 0xff).
2. After getting ECC error, erased CW detection logic will be
   applied which is different for BCH and RS ECC
    a. For BCH, HW checks if all the bytes in page are 0xff and then
       it updates the status in separate register
       NAND_ERASED_CW_DETECT_STATUS.
    b. For RS ECC, the HW reports the same error when reading an
       erased CW, but it notifies that it is an erased CW by
       placing special characters at certain offsets in the
       buffer.

So the erased CW detect status should be checked only if ECC engine
generated the uncorrectable error.

Currently for all other operational errors also (like TIMEOUT, MPU
errors, etc.), the erased CW detect logic is being applied so fix this
and return EIO for other operational errors.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

  1. Added more detail in commit message
  2. Added comment before each if/else
  3. Removed redundant check for BS_UNCORRECTABLE_BIT

 drivers/mtd/nand/raw/qcom_nandc.c | 65 ++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 3d1ff54..e6a21598 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1578,6 +1578,7 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
 	unsigned int max_bitflips = 0;
 	struct read_stats *buf;
+	bool flash_op_err = false;
 	int i;
 
 	buf = (struct read_stats *)nandc->reg_read_buf;
@@ -1599,8 +1600,18 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 		buffer = le32_to_cpu(buf->buffer);
 		erased_cw = le32_to_cpu(buf->erased_cw);
 
-		if (flash & (FS_OP_ERR | FS_MPU_ERR)) {
+		/*
+		 * Check ECC failure for each codeword. ECC failure can
+		 * happen in either of the following conditions
+		 * 1. If number of bitflips are greater than ECC engine
+		 *    capability.
+		 * 2. If this codeword contains all 0xff for which erased
+		 *    codeword detection check will be done.
+		 */
+		if ((flash & FS_OP_ERR) && (buffer & BS_UNCORRECTABLE_BIT)) {
 			bool erased;
+			int ret, ecclen, extraooblen;
+			void *eccbuf;
 
 			/* ignore erased codeword errors */
 			if (host->bch_enabled) {
@@ -1618,29 +1629,36 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 				continue;
 			}
 
-			if (buffer & BS_UNCORRECTABLE_BIT) {
-				int ret, ecclen, extraooblen;
-				void *eccbuf;
+			eccbuf = oob_buf ? oob_buf + oob_len : NULL;
+			ecclen = oob_buf ? host->ecc_bytes_hw : 0;
+			extraooblen = oob_buf ? oob_len : 0;
 
-				eccbuf = oob_buf ? oob_buf + oob_len : NULL;
-				ecclen = oob_buf ? host->ecc_bytes_hw : 0;
-				extraooblen = oob_buf ? oob_len : 0;
-
-				/*
-				 * make sure it isn't an erased page reported
-				 * as not-erased by HW because of a few bitflips
-				 */
-				ret = nand_check_erased_ecc_chunk(data_buf,
-					data_len, eccbuf, ecclen, oob_buf,
-					extraooblen, ecc->strength);
-				if (ret < 0) {
-					mtd->ecc_stats.failed++;
-				} else {
-					mtd->ecc_stats.corrected += ret;
-					max_bitflips =
-						max_t(unsigned int, max_bitflips, ret);
-				}
+			/*
+			 * make sure it isn't an erased page reported
+			 * as not-erased by HW because of a few bitflips
+			 */
+			ret = nand_check_erased_ecc_chunk(data_buf,
+				data_len, eccbuf, ecclen, oob_buf,
+				extraooblen, ecc->strength);
+			if (ret < 0) {
+				mtd->ecc_stats.failed++;
+			} else {
+				mtd->ecc_stats.corrected += ret;
+				max_bitflips =
+					max_t(unsigned int, max_bitflips, ret);
 			}
+		/*
+		 * Check if MPU or any other operational error (timeout,
+		 * device failure, etc.) happened for this codeword and
+		 * make flash_op_err true. If flash_op_err is set, then
+		 * EIO will be returned for page read.
+		 */
+		} else if (flash & (FS_OP_ERR | FS_MPU_ERR)) {
+			flash_op_err = true;
+		/*
+		 * No ECC or operational errors happened. Check the number of
+		 * bits corrected and update the ecc_stats.corrected.
+		 */
 		} else {
 			unsigned int stat;
 
@@ -1654,6 +1672,9 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 			oob_buf += oob_len + ecc->bytes;
 	}
 
+	if (flash_op_err)
+		return -EIO;
+
 	return max_bitflips;
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 07/14] mtd: rawnand: qcom: fix null pointer access for erased page detection

[Abhishek Sahu]

parse_read_errors can be called with only oob_buf in which case
data_buf will be NULL.  If data_buf is NULL, then don’t
treat this page as completely erased in case of ECC uncorrectable
error for RS ECC. For BCH ECC, the controller itself tells
regarding erased page in status register.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

  1. Added more detail in commit message
  2. Added comment before each if/else

 drivers/mtd/nand/raw/qcom_nandc.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index e6a21598..fa38142 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1613,13 +1613,24 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 			int ret, ecclen, extraooblen;
 			void *eccbuf;
 
-			/* ignore erased codeword errors */
+			/*
+			 * For BCH ECC, ignore erased codeword errors, if
+			 * ERASED_CW bits are set.
+			 */
 			if (host->bch_enabled) {
 				erased = (erased_cw & ERASED_CW) == ERASED_CW ?
 					 true : false;
-			} else {
+			/*
+			 * For RS ECC, HW reports the erased CW by placing
+			 * special characters at certain offsets in the buffer.
+			 * These special characters will be valid only if
+			 * complete page is read i.e. data_buf is not NULL.
+			 */
+			} else if (data_buf) {
 				erased = erased_chunk_check_and_fixup(data_buf,
 								      data_len);
+			} else {
+				erased = false;
 			}
 
 			if (erased) {
@@ -1667,7 +1678,8 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 			max_bitflips = max(max_bitflips, stat);
 		}
 
-		data_buf += data_len;
+		if (data_buf)
+			data_buf += data_len;
 		if (oob_buf)
 			oob_buf += oob_len + ecc->bytes;
 	}
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 08/14] mtd: rawnand: qcom: parse read errors for read oob also

[Abhishek Sahu]

read_page and read_oob both calls the read_page_ecc function.
The QCOM NAND controller protect the OOB available bytes with
ECC so read errors should be checked for read_oob also. Now
this patch moves the error checking code inside read_page_ecc
so caller does not have to check explicitly for read errors.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
* Changes from v1:

  1. Minor code change for return early in case of error

 drivers/mtd/nand/raw/qcom_nandc.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index fa38142..61d0e7d 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1700,6 +1700,7 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
 	struct nand_chip *chip = &host->chip;
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	u8 *data_buf_start = data_buf, *oob_buf_start = oob_buf;
 	int i, ret;
 
 	config_nand_page_read(nandc);
@@ -1760,12 +1761,14 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
 	}
 
 	ret = submit_descs(nandc);
-	if (ret)
+	free_descs(nandc);
+
+	if (ret) {
 		dev_err(nandc->dev, "failure to read page/oob\n");
+		return ret;
+	}
 
-	free_descs(nandc);
-
-	return ret;
+	return parse_read_errors(host, data_buf_start, oob_buf_start);
 }
 
 /*
@@ -1810,20 +1813,14 @@ static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	struct qcom_nand_host *host = to_qcom_nand_host(chip);
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	u8 *data_buf, *oob_buf = NULL;
-	int ret;
 
 	nand_read_page_op(chip, page, 0, NULL, 0);
 	data_buf = buf;
 	oob_buf = oob_required ? chip->oob_poi : NULL;
 
 	clear_bam_transaction(nandc);
-	ret = read_page_ecc(host, data_buf, oob_buf);
-	if (ret) {
-		dev_err(nandc->dev, "failure to read page\n");
-		return ret;
-	}
 
-	return parse_read_errors(host, data_buf, oob_buf);
+	return read_page_ecc(host, data_buf, oob_buf);
 }
 
 /* implements ecc->read_page_raw() */
@@ -1913,7 +1910,6 @@ static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 	struct qcom_nand_host *host = to_qcom_nand_host(chip);
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
-	int ret;
 
 	clear_read_regs(nandc);
 	clear_bam_transaction(nandc);
@@ -1922,11 +1918,7 @@ static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 	set_address(host, 0, page);
 	update_rw_regs(host, ecc->steps, true);
 
-	ret = read_page_ecc(host, NULL, chip->oob_poi);
-	if (ret)
-		dev_err(nandc->dev, "failure to read oob\n");
-
-	return ret;
+	return read_page_ecc(host, NULL, chip->oob_poi);
 }
 
 /* implements ecc->write_page() */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 09/14] mtd: rawnand: qcom: modify write_oob to remove read codeword part

[Abhishek Sahu]

QCOM NAND layout protect available OOB data bytes with ECC also so
when ecc->write_oob (qcom_nandc_write_oob) is being called then it
can't update just OOB bytes. Currently, it first reads the last
codeword which includes old OOB bytes. Then it updates the old OOB
bytes with new one and then again writes the codeword back.
The reading codeword is unnecessary since all the other bytes
should be 0xff only.

This patch removes the read part and updates the oob bytes with
all other data bytes as 0xff.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

 NEW CHANGE

 drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 61d0e7d..f85d8ab 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd,
  * implements ecc->write_oob()
  *
  * the NAND controller cannot write only data or only oob within a codeword,
- * since ecc is calculated for the combined codeword. we first copy the
- * entire contents for the last codeword(data + oob), replace the old oob
- * with the new one in chip->oob_poi, and then write the entire codeword.
- * this read-copy-write operation results in a slight performance loss.
+ * since ecc is calculated for the combined codeword. So make all the data
+ * bytes as 0xff and update the oob from chip->oob_poi, and then write
+ * the entire codeword again.
  */
 static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
 				int page)
@@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
 	int data_size, oob_size;
 	int ret;
 
-	host->use_ecc = true;
-
-	clear_bam_transaction(nandc);
-	ret = copy_last_cw(host, page);
-	if (ret)
-		return ret;
-
-	clear_read_regs(nandc);
 	clear_bam_transaction(nandc);
 
 	/* calculate the data and oob size for the last codeword/step */
 	data_size = ecc->size - ((ecc->steps - 1) << 2);
 	oob_size = mtd->oobavail;
+	host->use_ecc = true;
 
+	memset(nandc->data_buffer, 0xff, host->cw_data);
 	/* override new oob content to last codeword */
 	mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, oob,
 				    0, mtd->oobavail);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 10/14] mtd: rawnand: qcom: fix return value for raw page read

[Abhishek Sahu]

Currently zero is being returned for all raw page read so
fix the same.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

 NEW CHANGE

 drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index f85d8ab..17b7f3af 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1900,7 +1900,7 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,
 
 	free_descs(nandc);
 
-	return 0;
+	return ret;
 }
 
 /* implements ecc->read_oob() */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 11/14] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

The QCOM NAND layout is such that, the bad block byte offset for
last codeword will come to first byte in spare area. Currently,
the raw read for last codeword is being done with copy_last_cw
function. It does following 2 things

1. Read the last codeword bytes from NAND chip to NAND
   controller internal HW buffer.
2. Copy all these bytes from this HW buffer to driver RAM
   buffer.

For bad block check, maximum two bytes are required so instead of
copying the complete bytes in step 2, only those bbm_size bytes
can be copied.

This patch does minor code reorganization for the same. After
this, copy_last_cw function won’t be required.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

 NEW CHANGE

 drivers/mtd/nand/raw/qcom_nandc.c | 66 +++++++++++++++------------------------
 1 file changed, 25 insertions(+), 41 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 17b7f3af..10bdc6c 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1771,41 +1771,6 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
 	return parse_read_errors(host, data_buf_start, oob_buf_start);
 }
 
-/*
- * a helper that copies the last step/codeword of a page (containing free oob)
- * into our local buffer
- */
-static int copy_last_cw(struct qcom_nand_host *host, int page)
-{
-	struct nand_chip *chip = &host->chip;
-	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
-	struct nand_ecc_ctrl *ecc = &chip->ecc;
-	int size;
-	int ret;
-
-	clear_read_regs(nandc);
-
-	size = host->use_ecc ? host->cw_data : host->cw_size;
-
-	/* prepare a clean read buffer */
-	memset(nandc->data_buffer, 0xff, size);
-
-	set_address(host, host->cw_size * (ecc->steps - 1), page);
-	update_rw_regs(host, 1, true);
-
-	config_nand_single_cw_page_read(nandc);
-
-	read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0);
-
-	ret = submit_descs(nandc);
-	if (ret)
-		dev_err(nandc->dev, "failed to copy last codeword\n");
-
-	free_descs(nandc);
-
-	return ret;
-}
-
 /* implements ecc->read_page() */
 static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 				uint8_t *buf, int oob_required, int page)
@@ -2121,6 +2086,7 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
 	int page, ret, bbpos, bad = 0;
 	u32 flash_status;
+	u8 *bbm_bytes_buf = chip->data_buf;
 
 	page = (int)(ofs >> chip->page_shift) & chip->pagemask;
 
@@ -2131,11 +2097,31 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)
 	 * that contains the BBM
 	 */
 	host->use_ecc = false;
+	bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1);
 
 	clear_bam_transaction(nandc);
-	ret = copy_last_cw(host, page);
-	if (ret)
+	clear_read_regs(nandc);
+
+	set_address(host, host->cw_size * (ecc->steps - 1), page);
+	update_rw_regs(host, 1, true);
+
+	/*
+	 * The last codeword data will be copied from NAND device to NAND
+	 * controller internal HW buffer. Copy only required BBM size bytes
+	 * from this HW buffer to bbm_bytes_buf which is present at
+	 * bbpos offset.
+	 */
+	nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);
+	config_nand_single_cw_page_read(nandc);
+	read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf,
+		      host->bbm_size, 0);
+
+	ret = submit_descs(nandc);
+	free_descs(nandc);
+	if (ret) {
+		dev_err(nandc->dev, "failed to copy bad block bytes\n");
 		goto err;
+	}
 
 	flash_status = le32_to_cpu(nandc->reg_read_buf[0]);
 
@@ -2144,12 +2130,10 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)
 		goto err;
 	}
 
-	bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1);
-
-	bad = nandc->data_buffer[bbpos] != 0xff;
+	bad = bbm_bytes_buf[0] != 0xff;
 
 	if (chip->options & NAND_BUSWIDTH_16)
-		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
+		bad = bad || (bbm_bytes_buf[1] != 0xff);
 err:
 	return bad;
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 12/14] mtd: rawnand: qcom: check for operation errors in case of raw read

[Abhishek Sahu]

Currently there is no error checking for raw read. For raw
reads, there won’t be any ECC failure but the operational
failures are possible so schedule the NAND_FLASH_STATUS read
after each codeword.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

 1. Removed the code for copy_last_cw.

 drivers/mtd/nand/raw/qcom_nandc.c | 58 +++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 10bdc6c..d828115 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1099,7 +1099,8 @@ static void config_nand_page_read(struct qcom_nand_controller *nandc)
  * Helper to prepare DMA descriptors for configuring registers
  * before reading each codeword in NAND page.
  */
-static void config_nand_cw_read(struct qcom_nand_controller *nandc)
+static void
+config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
 {
 	if (nandc->props->is_bam)
 		write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
@@ -1108,19 +1109,25 @@ static void config_nand_cw_read(struct qcom_nand_controller *nandc)
 	write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
 	write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
 
-	read_reg_dma(nandc, NAND_FLASH_STATUS, 2, 0);
-	read_reg_dma(nandc, NAND_ERASED_CW_DETECT_STATUS, 1,
-		     NAND_BAM_NEXT_SGL);
+	if (use_ecc) {
+		read_reg_dma(nandc, NAND_FLASH_STATUS, 2, 0);
+		read_reg_dma(nandc, NAND_ERASED_CW_DETECT_STATUS, 1,
+			     NAND_BAM_NEXT_SGL);
+	} else {
+		read_reg_dma(nandc, NAND_FLASH_STATUS, 1, NAND_BAM_NEXT_SGL);
+	}
 }
 
 /*
  * Helper to prepare dma descriptors to configure registers needed for reading a
  * single codeword in page
  */
-static void config_nand_single_cw_page_read(struct qcom_nand_controller *nandc)
+static void
+config_nand_single_cw_page_read(struct qcom_nand_controller *nandc,
+				bool use_ecc)
 {
 	config_nand_page_read(nandc);
-	config_nand_cw_read(nandc);
+	config_nand_cw_read(nandc, use_ecc);
 }
 
 /*
@@ -1201,7 +1208,7 @@ static int nandc_param(struct qcom_nand_host *host)
 	nandc->buf_count = 512;
 	memset(nandc->data_buffer, 0xff, nandc->buf_count);
 
-	config_nand_single_cw_page_read(nandc);
+	config_nand_single_cw_page_read(nandc, false);
 
 	read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer,
 		      nandc->buf_count, 0);
@@ -1565,6 +1572,23 @@ struct read_stats {
 	__le32 erased_cw;
 };
 
+/* reads back FLASH_STATUS register set by the controller */
+static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
+{
+	struct nand_chip *chip = &host->chip;
+	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+	int i;
+
+	for (i = 0; i < cw_cnt; i++) {
+		u32 flash = le32_to_cpu(nandc->reg_read_buf[i]);
+
+		if (flash & (FS_OP_ERR | FS_MPU_ERR))
+			return -EIO;
+	}
+
+	return 0;
+}
+
 /*
  * reads back status registers set by the controller to notify page read
  * errors. this is equivalent to what 'ecc->correct()' would do.
@@ -1731,7 +1755,7 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
 			}
 		}
 
-		config_nand_cw_read(nandc);
+		config_nand_cw_read(nandc, true);
 
 		if (data_buf)
 			read_data_dma(nandc, FLASH_BUF_ACC, data_buf,
@@ -1841,7 +1865,7 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,
 			nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
 		}
 
-		config_nand_cw_read(nandc);
+		config_nand_cw_read(nandc, false);
 
 		read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
 		reg_off += data_size1;
@@ -1860,12 +1884,13 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,
 	}
 
 	ret = submit_descs(nandc);
-	if (ret)
+	free_descs(nandc);
+	if (ret) {
 		dev_err(nandc->dev, "failure to read raw page\n");
+		return ret;
+	}
 
-	free_descs(nandc);
-
-	return ret;
+	return check_flash_errors(host, ecc->steps);
 }
 
 /* implements ecc->read_oob() */
@@ -2085,7 +2110,6 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
 	int page, ret, bbpos, bad = 0;
-	u32 flash_status;
 	u8 *bbm_bytes_buf = chip->data_buf;
 
 	page = (int)(ofs >> chip->page_shift) & chip->pagemask;
@@ -2112,7 +2136,7 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)
 	 * bbpos offset.
 	 */
 	nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);
-	config_nand_single_cw_page_read(nandc);
+	config_nand_single_cw_page_read(nandc, host->use_ecc);
 	read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf,
 		      host->bbm_size, 0);
 
@@ -2123,9 +2147,7 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)
 		goto err;
 	}
 
-	flash_status = le32_to_cpu(nandc->reg_read_buf[0]);
-
-	if (flash_status & (FS_OP_ERR | FS_MPU_ERR)) {
+	if (check_flash_errors(host, 1)) {
 		dev_warn(nandc->dev, "error when trying to read BBM\n");
 		goto err;
 	}
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 13/14] mtd: rawnand: qcom: helper function for raw read

[Abhishek Sahu]

This patch does minor code reorganization for raw reads.
Currently the raw read is required for complete page but for
subsequent patches related with erased codeword bit flips
detection, only few CW should be read. So, this patch adds
helper function and introduces the read CW bitmask which
specifies which CW reads are required in complete page.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

 1. Included more detail in function comment

 drivers/mtd/nand/raw/qcom_nandc.c | 197 ++++++++++++++++++++++++--------------
 1 file changed, 123 insertions(+), 74 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index d828115..5148b49 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1590,6 +1590,127 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
 }
 
 /*
+ * Helper to perform the page raw read operation. The read_cw_mask will be
+ * used to specify the codewords (CW) for which the data should be read. The
+ * single page contains multiple CW.
+ *
+ * Normally other NAND controllers store the data in main area and
+ * ecc bytes in oob area. So, if page size is 2048+64 then 2048
+ * data bytes will go in main area followed by ECC bytes. The QCOM NAND
+ * controller follows different layout in which the data+oob is internally
+ * divided in 528/532 bytes CW and each CW contains 516 bytes followed by
+ * ECC parity bytes for that CW. By this, 4 available oob bytes per CW
+ * will also be protected with ECC.
+ *
+ * For each CW read, following are the 2 steps
+ * 1. Read the codeword bytes from NAND chip to NAND controller internal HW
+ *    buffer.
+ * 2. Copy all these bytes from this HW buffer to driver RAM buffer.
+ *
+ * Sometime, only few CW data is required in complete page. The read_cw_mask
+ * specifies which CW in a page needs to be read. Start address will be
+ * determined with this CW mask to skip unnecessary data copy from NAND
+ * flash device. Then, actual data copy from NAND controller HW internal buffer
+ * to data buffer will be done only for the CWs, which have the mask set.
+ */
+static int
+nandc_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+		    u8 *data_buf, u8 *oob_buf,
+		    int page, unsigned long read_cw_mask)
+{
+	struct qcom_nand_host *host = to_qcom_nand_host(chip);
+	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int i, ret;
+	int read_loc, start_step, last_step;
+
+	nand_read_page_op(chip, page, 0, NULL, 0);
+
+	host->use_ecc = false;
+	start_step = ffs(read_cw_mask) - 1;
+	last_step = fls(read_cw_mask);
+
+	clear_bam_transaction(nandc);
+	set_address(host, host->cw_size * start_step, page);
+	update_rw_regs(host, last_step - start_step, true);
+	config_nand_page_read(nandc);
+
+	for (i = start_step; i < last_step; i++) {
+		int data_size1, data_size2, oob_size1, oob_size2;
+		int reg_off = FLASH_BUF_ACC;
+
+		data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
+		oob_size1 = host->bbm_size;
+
+		if (i == (ecc->steps - 1)) {
+			data_size2 = ecc->size - data_size1 -
+				     ((ecc->steps - 1) << 2);
+			oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +
+				    host->spare_bytes;
+		} else {
+			data_size2 = host->cw_data - data_size1;
+			oob_size2 = host->ecc_bytes_hw + host->spare_bytes;
+		}
+
+		/*
+		 * Don't perform actual data copy from NAND controller internal
+		 * HW buffer to data buffer through DMA for this codeword.
+		 */
+		if (!(read_cw_mask & BIT(i))) {
+			if (nandc->props->is_bam)
+				nandc_set_read_loc(nandc, 0, 0, 0, 1);
+
+			config_nand_cw_read(nandc, false);
+
+			data_buf += data_size1 + data_size2;
+			oob_buf += oob_size1 + oob_size2;
+
+			continue;
+		}
+
+		if (nandc->props->is_bam) {
+			read_loc = 0;
+			nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
+			read_loc += data_size1;
+
+			nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
+			read_loc += oob_size1;
+
+			nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
+			read_loc += data_size2;
+
+			nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
+		}
+
+		config_nand_cw_read(nandc, false);
+
+		read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
+		reg_off += data_size1;
+		data_buf += data_size1;
+
+		read_data_dma(nandc, reg_off, oob_buf, oob_size1, 0);
+		reg_off += oob_size1;
+		oob_buf += oob_size1;
+
+		read_data_dma(nandc, reg_off, data_buf, data_size2, 0);
+		reg_off += data_size2;
+		data_buf += data_size2;
+
+		read_data_dma(nandc, reg_off, oob_buf, oob_size2, 0);
+		oob_buf += oob_size2;
+	}
+
+	ret = submit_descs(nandc);
+	free_descs(nandc);
+	if (ret) {
+		dev_err(nandc->dev, "failure to read raw page\n");
+		return ret;
+	}
+
+	return check_flash_errors(host, ecc->steps);
+}
+
+/*
  * reads back status registers set by the controller to notify page read
  * errors. this is equivalent to what 'ecc->correct()' would do.
  */
@@ -1817,80 +1938,8 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,
 				    struct nand_chip *chip, uint8_t *buf,
 				    int oob_required, int page)
 {
-	struct qcom_nand_host *host = to_qcom_nand_host(chip);
-	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
-	u8 *data_buf, *oob_buf;
-	struct nand_ecc_ctrl *ecc = &chip->ecc;
-	int i, ret;
-	int read_loc;
-
-	nand_read_page_op(chip, page, 0, NULL, 0);
-	data_buf = buf;
-	oob_buf = chip->oob_poi;
-
-	host->use_ecc = false;
-
-	clear_bam_transaction(nandc);
-	update_rw_regs(host, ecc->steps, true);
-	config_nand_page_read(nandc);
-
-	for (i = 0; i < ecc->steps; i++) {
-		int data_size1, data_size2, oob_size1, oob_size2;
-		int reg_off = FLASH_BUF_ACC;
-
-		data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
-		oob_size1 = host->bbm_size;
-
-		if (i == (ecc->steps - 1)) {
-			data_size2 = ecc->size - data_size1 -
-				     ((ecc->steps - 1) << 2);
-			oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +
-				    host->spare_bytes;
-		} else {
-			data_size2 = host->cw_data - data_size1;
-			oob_size2 = host->ecc_bytes_hw + host->spare_bytes;
-		}
-
-		if (nandc->props->is_bam) {
-			read_loc = 0;
-			nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
-			read_loc += data_size1;
-
-			nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
-			read_loc += oob_size1;
-
-			nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
-			read_loc += data_size2;
-
-			nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
-		}
-
-		config_nand_cw_read(nandc, false);
-
-		read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
-		reg_off += data_size1;
-		data_buf += data_size1;
-
-		read_data_dma(nandc, reg_off, oob_buf, oob_size1, 0);
-		reg_off += oob_size1;
-		oob_buf += oob_size1;
-
-		read_data_dma(nandc, reg_off, data_buf, data_size2, 0);
-		reg_off += data_size2;
-		data_buf += data_size2;
-
-		read_data_dma(nandc, reg_off, oob_buf, oob_size2, 0);
-		oob_buf += oob_size2;
-	}
-
-	ret = submit_descs(nandc);
-	free_descs(nandc);
-	if (ret) {
-		dev_err(nandc->dev, "failure to read raw page\n");
-		return ret;
-	}
-
-	return check_flash_errors(host, ecc->steps);
+	return nandc_read_page_raw(mtd, chip, buf, chip->oob_poi, page,
+				   BIT(chip->ecc.steps) - 1);
 }
 
 /* implements ecc->read_oob() */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 14/14] mtd: rawnand: qcom: erased page bitflips detection

[Abhishek Sahu]

NAND parts can have bitflips in an erased page due to the
process technology used. In this case, qpic nand controller
is not able to identify that page as an erased page.
Currently the driver calls nand_check_erased_ecc_chunk for
identifying the erased pages but this won’t work always since the
checking is being with ECC engine returned data. In case of
bitflips, the ECC engine tries to correct the data and then it
generates the uncorrectable error. Now, this data is not equal to
original raw data. For erased CW identification, the raw data
should be read again from NAND device and this
nand_check_erased_ecc_chunk function should be called for raw
data only.

Now following logic is being added to identify the erased
codeword bitflips.

1. In most of the case, not all the codewords will have bitflips
   and only single CW will have bitflips. So, there is no need to
   read the complete raw page data. The NAND raw read can be
   scheduled for any CW in page. The NAND controller works on CW
   basis and it will update the status register after each CW read.
   Maintain the bitmask for the CW which generated the uncorrectable
   error.
2. Schedule the raw flash read from NAND flash device to
   NAND controller buffer for all these CWs between first and last
   uncorrectable errors CWs. Copy the content from NAND controller
   internal HW buffer to actual data buffer only for the uncorrectable
   errors CWs so that other CW data content won’t be affected, and
   unnecessary data copy can be avoided.
3. Both DATA and OOB need to be checked for number of 0. The
   top-level API can be called with only data buf or oob buf so use
   chip->databuf if data buf is null and chip->oob_poi if
   oob buf is null for copying the raw bytes temporarily.
4. For each CW, check the number of 0 in cw_data and usable
   oob bytes, The bbm and spare (unused) bytes bit flip won’t
   affect the ECC so don’t check the number of bitflips in this area.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v1:

 1. Minor change in commit message
 2. invalidate pagebuf if databuf or oob_poi is used

 drivers/mtd/nand/raw/qcom_nandc.c | 135 +++++++++++++++++++++++++++-----------
 1 file changed, 98 insertions(+), 37 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 5148b49..d63ea92 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1711,20 +1711,103 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
 }
 
 /*
+ * Bitflips can happen in erased codewords also so this function counts the
+ * number of 0 in each CW for which ECC engine returns the uncorrectable
+ * error. The page will be assumed as erased if this count is less than or
+ * equal to the ecc->strength for each CW.
+ *
+ * 1. Both DATA and OOB need to be checked for number of 0. The
+ *    top-level API can be called with only data buf or oob buf so use
+ *    chip->data_buf if data buf is null and chip->oob_poi if oob buf
+ *    is null for copying the raw bytes.
+ * 2. Perform raw read for all the CW which has uncorrectable errors.
+ * 3. For each CW, check the number of 0 in cw_data and usable oob bytes.
+ *    The bbm and spare bytes bit flip won’t affect the ECC so don’t check
+ *    the number of bitflips in this area.
+ */
+static int
+check_for_erased_page(struct qcom_nand_host *host, u8 *data_buf,
+		      u8 *oob_buf, unsigned long uncorrectable_err_cws,
+		      int page, unsigned int max_bitflips)
+{
+	struct nand_chip *chip = &host->chip;
+	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int i, start_step, last_step, ret = 0;
+
+	start_step = ffs(uncorrectable_err_cws) - 1;
+	last_step = fls(uncorrectable_err_cws);
+
+	if (!data_buf) {
+		data_buf = chip->data_buf;
+		chip->pagebuf = -1;
+	}
+
+	if (!oob_buf) {
+		oob_buf = chip->oob_poi;
+		chip->pagebuf = -1;
+	}
+
+	data_buf += start_step * host->cw_data;
+	oob_buf += start_step * ecc->bytes;
+
+	clear_read_regs(nandc);
+	nandc_read_page_raw(mtd, chip, data_buf, oob_buf, page,
+			    uncorrectable_err_cws);
+
+	for (i = start_step; i < last_step; i++) {
+		int data_size, oob_size;
+
+		if (i == (ecc->steps - 1)) {
+			data_size = ecc->size - ((ecc->steps - 1) << 2);
+			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw;
+		} else {
+			data_size = host->cw_data;
+			oob_size = host->ecc_bytes_hw;
+		}
+
+		if (uncorrectable_err_cws & BIT(i)) {
+			/*
+			 * make sure it isn't an erased page reported
+			 * as not-erased by HW because of a few bitflips
+			 */
+			ret = nand_check_erased_ecc_chunk(data_buf,
+				data_size, oob_buf + host->bbm_size,
+				oob_size, NULL,
+				0, ecc->strength);
+			if (ret < 0) {
+				mtd->ecc_stats.failed++;
+			} else {
+				mtd->ecc_stats.corrected += ret;
+				max_bitflips =
+					max_t(unsigned int, max_bitflips, ret);
+			}
+		}
+
+		data_buf += data_size;
+		oob_buf += ecc->bytes;
+	}
+
+	return max_bitflips;
+}
+
+/*
  * reads back status registers set by the controller to notify page read
  * errors. this is equivalent to what 'ecc->correct()' would do.
  */
 static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
-			     u8 *oob_buf)
+			     u8 *oob_buf, int page)
 {
 	struct nand_chip *chip = &host->chip;
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
-	unsigned int max_bitflips = 0;
+	unsigned int max_bitflips = 0, uncorrectable_err_cws = 0;
 	struct read_stats *buf;
-	bool flash_op_err = false;
+	bool flash_op_err = false, erased;
 	int i;
+	u8 *data_buf_start = data_buf, *oob_buf_start = oob_buf;
 
 	buf = (struct read_stats *)nandc->reg_read_buf;
 	nandc_read_buffer_sync(nandc, true);
@@ -1754,10 +1837,6 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 		 *    codeword detection check will be done.
 		 */
 		if ((flash & FS_OP_ERR) && (buffer & BS_UNCORRECTABLE_BIT)) {
-			bool erased;
-			int ret, ecclen, extraooblen;
-			void *eccbuf;
-
 			/*
 			 * For BCH ECC, ignore erased codeword errors, if
 			 * ERASED_CW bits are set.
@@ -1778,31 +1857,8 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 				erased = false;
 			}
 
-			if (erased) {
-				data_buf += data_len;
-				if (oob_buf)
-					oob_buf += oob_len + ecc->bytes;
-				continue;
-			}
-
-			eccbuf = oob_buf ? oob_buf + oob_len : NULL;
-			ecclen = oob_buf ? host->ecc_bytes_hw : 0;
-			extraooblen = oob_buf ? oob_len : 0;
-
-			/*
-			 * make sure it isn't an erased page reported
-			 * as not-erased by HW because of a few bitflips
-			 */
-			ret = nand_check_erased_ecc_chunk(data_buf,
-				data_len, eccbuf, ecclen, oob_buf,
-				extraooblen, ecc->strength);
-			if (ret < 0) {
-				mtd->ecc_stats.failed++;
-			} else {
-				mtd->ecc_stats.corrected += ret;
-				max_bitflips =
-					max_t(unsigned int, max_bitflips, ret);
-			}
+			if (!erased)
+				uncorrectable_err_cws |= BIT(i);
 		/*
 		 * Check if MPU or any other operational error (timeout,
 		 * device failure, etc.) happened for this codeword and
@@ -1832,7 +1888,12 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
 	if (flash_op_err)
 		return -EIO;
 
-	return max_bitflips;
+	if (!uncorrectable_err_cws)
+		return max_bitflips;
+
+	return check_for_erased_page(host, data_buf_start, oob_buf_start,
+				     uncorrectable_err_cws, page,
+				     max_bitflips);
 }
 
 /*
@@ -1840,7 +1901,7 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
  * ecc->read_oob()
  */
 static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
-			 u8 *oob_buf)
+			 u8 *oob_buf, int page)
 {
 	struct nand_chip *chip = &host->chip;
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
@@ -1913,7 +1974,7 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
 		return ret;
 	}
 
-	return parse_read_errors(host, data_buf_start, oob_buf_start);
+	return parse_read_errors(host, data_buf_start, oob_buf_start, page);
 }
 
 /* implements ecc->read_page() */
@@ -1930,7 +1991,7 @@ static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 
 	clear_bam_transaction(nandc);
 
-	return read_page_ecc(host, data_buf, oob_buf);
+	return read_page_ecc(host, data_buf, oob_buf, page);
 }
 
 /* implements ecc->read_page_raw() */
@@ -1957,7 +2018,7 @@ static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 	set_address(host, 0, page);
 	update_rw_regs(host, ecc->steps, true);
 
-	return read_page_ecc(host, NULL, chip->oob_poi);
+	return read_page_ecc(host, NULL, chip->oob_poi, page);
 }
 
 /* implements ecc->write_page() */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters

[Masahiro Yamada]

2018-05-03 21:20 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
> commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
> match, maximize ECC settings") provides generic helpers which
> drivers can use for setting up ECC parameters.
>
> Since same board can have different ECC strength nand chips so
> following is the logic for setting up ECC strength and ECC step
> size, which can be used by most of the drivers.
>
> 1. If both ECC step size and ECC strength are already set
>    (usually by DT) then just check whether this setting
>    is supported by NAND controller.
> 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
>    supported by NAND controller.
> 3. Otherwise, try to match the ECC step size and ECC strength closest
>    to the chip's requirement. If available OOB size can't fit the chip
>    requirement then select maximum ECC strength which can be fit with
>    available OOB size with warning.
>
> This patch introduces nand_ecc_param_setup function which calls the
> required helper functions for the above logic. The drivers can use
> this single function instead of calling the 3 helper functions
> individually.
>
> CC: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
>
>   NEW PATCH
>
>  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/rawnand.h      |  3 +++
>  2 files changed, 45 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..dd7a984 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
>  }
>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>
> +/**
> + * nand_ecc_param_setup - Set the ECC strength and ECC step size
> + * @chip: nand chip info structure
> + * @caps: ECC engine caps info structure
> + * @oobavail: OOB size that the ECC engine can use
> + *
> + * Choose the ECC strength according to following logic
> + *
> + * 1. If both ECC step size and ECC strength are already set (usually by DT)
> + *    then check if it is supported by this controller.
> + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
> + * 3. Otherwise, try to match the ECC step size and ECC strength closest
> + *    to the chip's requirement. If available OOB size can't fit the chip
> + *    requirement then fallback to the maximum ECC step size and ECC strength
> + *    and print the warning.
> + *
> + * On success, the chosen ECC settings are set.
> + */
> +int nand_ecc_param_setup(struct nand_chip *chip,
> +                        const struct nand_ecc_caps *caps, int oobavail)
> +{
> +       int ret;
> +
> +       if (chip->ecc.size && chip->ecc.strength)
> +               return nand_check_ecc_caps(chip, caps, oobavail);
> +
> +       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
> +               return nand_maximize_ecc(chip, caps, oobavail);
> +
> +       if (!nand_match_ecc_req(chip, caps, oobavail))
> +               return 0;
> +
> +       ret = nand_maximize_ecc(chip, caps, oobavail);


Why two calls for nand_maximize_ecc()?

My code is simpler, and does not display
false-positive warning.


> +       if (!ret)
> +               pr_warn("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n",
> +                       chip->ecc_step_ds, chip->ecc_strength_ds,
> +                       chip->ecc.size, chip->ecc.strength);


This is annoying.

{ecc_step_ds, ecc_strength_ds} are not provided by Non-ONFi devices.

So,
  ECC (step, strength) = (0, 0) not supported on this controller.

will be always displayed.


The strength will be checked by nand_ecc_strength_good() anyway.






> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(nand_ecc_param_setup);



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters

[Abhishek Sahu]

On 2018-05-07 09:10, Masahiro Yamada wrote:
> 2018-05-03 21:20 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
>> commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
>> match, maximize ECC settings") provides generic helpers which
>> drivers can use for setting up ECC parameters.
>> 
>> Since same board can have different ECC strength nand chips so
>> following is the logic for setting up ECC strength and ECC step
>> size, which can be used by most of the drivers.
>> 
>> 1. If both ECC step size and ECC strength are already set
>>    (usually by DT) then just check whether this setting
>>    is supported by NAND controller.
>> 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
>>    supported by NAND controller.
>> 3. Otherwise, try to match the ECC step size and ECC strength closest
>>    to the chip's requirement. If available OOB size can't fit the chip
>>    requirement then select maximum ECC strength which can be fit with
>>    available OOB size with warning.
>> 
>> This patch introduces nand_ecc_param_setup function which calls the
>> required helper functions for the above logic. The drivers can use
>> this single function instead of calling the 3 helper functions
>> individually.
>> 
>> CC: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v1:
>> 
>>   NEW PATCH
>> 
>>  drivers/mtd/nand/raw/nand_base.c | 42 
>> ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/rawnand.h      |  3 +++
>>  2 files changed, 45 insertions(+)
>> 
>> diff --git a/drivers/mtd/nand/raw/nand_base.c 
>> b/drivers/mtd/nand/raw/nand_base.c
>> index 72f3a89..dd7a984 100644
>> --- a/drivers/mtd/nand/raw/nand_base.c
>> +++ b/drivers/mtd/nand/raw/nand_base.c
>> @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
>>  }
>>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>> 
>> +/**
>> + * nand_ecc_param_setup - Set the ECC strength and ECC step size
>> + * @chip: nand chip info structure
>> + * @caps: ECC engine caps info structure
>> + * @oobavail: OOB size that the ECC engine can use
>> + *
>> + * Choose the ECC strength according to following logic
>> + *
>> + * 1. If both ECC step size and ECC strength are already set (usually 
>> by DT)
>> + *    then check if it is supported by this controller.
>> + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
>> + * 3. Otherwise, try to match the ECC step size and ECC strength 
>> closest
>> + *    to the chip's requirement. If available OOB size can't fit the 
>> chip
>> + *    requirement then fallback to the maximum ECC step size and ECC 
>> strength
>> + *    and print the warning.
>> + *
>> + * On success, the chosen ECC settings are set.
>> + */
>> +int nand_ecc_param_setup(struct nand_chip *chip,
>> +                        const struct nand_ecc_caps *caps, int 
>> oobavail)
>> +{
>> +       int ret;
>> +
>> +       if (chip->ecc.size && chip->ecc.strength)
>> +               return nand_check_ecc_caps(chip, caps, oobavail);
>> +
>> +       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>> +               return nand_maximize_ecc(chip, caps, oobavail);
>> +
>> +       if (!nand_match_ecc_req(chip, caps, oobavail))
>> +               return 0;
>> +
>> +       ret = nand_maximize_ecc(chip, caps, oobavail);
> 
> 
> Why two calls for nand_maximize_ecc()?
> 
> My code is simpler, and does not display
> false-positive warning.
> 

  Thanks Masahiro.

  Since, Now this is in moved to generic layer that's why I put
  this warning that this function is falling back to some
  other ECC settings which is not recommend by chip.

  If this warning seems unnecessary then I can remove this
  and then directly your code changes can be put here
  instead of calling nand_maximize_ecc 2 times.

> 
>> +       if (!ret)
>> +               pr_warn("ECC (step, strength) = (%d, %d) not supported 
>> on this controller. Fallback to (%d, %d)\n",
>> +                       chip->ecc_step_ds, chip->ecc_strength_ds,
>> +                       chip->ecc.size, chip->ecc.strength);
> 
> 
> This is annoying.
> 
> {ecc_step_ds, ecc_strength_ds} are not provided by Non-ONFi devices.
> 
> So,
>   ECC (step, strength) = (0, 0) not supported on this controller.
> 
> will be always displayed.
> 
> 
> The strength will be checked by nand_ecc_strength_good() anyway.
> 

  But for most of the non ONFI devices also, this is being calculated
  by ID.

  You can get some background for this in
  http://lists.infradead.org/pipermail/linux-mtd/2018-April/080193.html

  Regards,
  Abhishek

Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters

[Boris Brezillon]

On Mon, 7 May 2018 12:40:39 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> 2018-05-03 21:20 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
> > commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
> > match, maximize ECC settings") provides generic helpers which
> > drivers can use for setting up ECC parameters.
> >
> > Since same board can have different ECC strength nand chips so
> > following is the logic for setting up ECC strength and ECC step
> > size, which can be used by most of the drivers.
> >
> > 1. If both ECC step size and ECC strength are already set
> >    (usually by DT) then just check whether this setting
> >    is supported by NAND controller.
> > 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
> >    supported by NAND controller.
> > 3. Otherwise, try to match the ECC step size and ECC strength closest
> >    to the chip's requirement. If available OOB size can't fit the chip
> >    requirement then select maximum ECC strength which can be fit with
> >    available OOB size with warning.
> >
> > This patch introduces nand_ecc_param_setup function which calls the
> > required helper functions for the above logic. The drivers can use
> > this single function instead of calling the 3 helper functions
> > individually.
> >
> > CC: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> > ---
> > * Changes from v1:
> >
> >   NEW PATCH
> >
> >  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/rawnand.h      |  3 +++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..dd7a984 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
> >  }
> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
> >
> > +/**
> > + * nand_ecc_param_setup - Set the ECC strength and ECC step size
> > + * @chip: nand chip info structure
> > + * @caps: ECC engine caps info structure
> > + * @oobavail: OOB size that the ECC engine can use
> > + *
> > + * Choose the ECC strength according to following logic
> > + *
> > + * 1. If both ECC step size and ECC strength are already set (usually by DT)
> > + *    then check if it is supported by this controller.
> > + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
> > + * 3. Otherwise, try to match the ECC step size and ECC strength closest
> > + *    to the chip's requirement. If available OOB size can't fit the chip
> > + *    requirement then fallback to the maximum ECC step size and ECC strength
> > + *    and print the warning.
> > + *
> > + * On success, the chosen ECC settings are set.
> > + */
> > +int nand_ecc_param_setup(struct nand_chip *chip,
> > +                        const struct nand_ecc_caps *caps, int oobavail)
> > +{
> > +       int ret;
> > +
> > +       if (chip->ecc.size && chip->ecc.strength)
> > +               return nand_check_ecc_caps(chip, caps, oobavail);
> > +
> > +       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
> > +               return nand_maximize_ecc(chip, caps, oobavail);
> > +
> > +       if (!nand_match_ecc_req(chip, caps, oobavail))
> > +               return 0;
> > +
> > +       ret = nand_maximize_ecc(chip, caps, oobavail);  
> 
> 
> Why two calls for nand_maximize_ecc()?

As long as the code does the same thing, I don't care that much.

> 
> My code is simpler,

and I don't see how your code is simpler. Mainly a matter of taste
AFAICS.

> and does not display
> false-positive warning.

I agree on the false-positive warning though, this should be avoided.

> 
> 
> > +       if (!ret)
> > +               pr_warn("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n",
> > +                       chip->ecc_step_ds, chip->ecc_strength_ds,
> > +                       chip->ecc.size, chip->ecc.strength);  
> 
> 
> This is annoying.
> 
> {ecc_step_ds, ecc_strength_ds} are not provided by Non-ONFi devices.
> 
> So,
>   ECC (step, strength) = (0, 0) not supported on this controller.

Well, if you have a chip that requires ECC but exposes 0bits/0bytes
then this should be fixed. 0,0 should only be valid when the chip does
not require ECC at all (so, only really old chips). For all other chips,
including non-ONFI ones, we should have a valid value here.

> 
> will be always displayed.
> 
> 
> The strength will be checked by nand_ecc_strength_good() anyway.

True. So, I agree that the pr_warn() is unneeded, but I still think we
should fix all cases where ECC reqs are missing, so if you have such a
setup, please add some code to nand_<vendor>.c to initialize
->ecc_xxx_ds properly.

Thanks,

Boris

Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters

[Boris Brezillon]

On Thu,  3 May 2018 17:50:28 +0530
Abhishek Sahu <absahu@codeaurora.org> wrote:

> commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
> match, maximize ECC settings") provides generic helpers which
> drivers can use for setting up ECC parameters.
> 
> Since same board can have different ECC strength nand chips so
> following is the logic for setting up ECC strength and ECC step
> size, which can be used by most of the drivers.
> 
> 1. If both ECC step size and ECC strength are already set
>    (usually by DT) then just check whether this setting
>    is supported by NAND controller.
> 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
>    supported by NAND controller.
> 3. Otherwise, try to match the ECC step size and ECC strength closest
>    to the chip's requirement. If available OOB size can't fit the chip
>    requirement then select maximum ECC strength which can be fit with
>    available OOB size with warning.
> 
> This patch introduces nand_ecc_param_setup function which calls the
> required helper functions for the above logic. The drivers can use
> this single function instead of calling the 3 helper functions
> individually.
> 
> CC: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
> 
>   NEW PATCH
> 
>  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/rawnand.h      |  3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..dd7a984 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
>  }
>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>  
> +/**
> + * nand_ecc_param_setup - Set the ECC strength and ECC step size
> + * @chip: nand chip info structure
> + * @caps: ECC engine caps info structure
> + * @oobavail: OOB size that the ECC engine can use
> + *
> + * Choose the ECC strength according to following logic
> + *
> + * 1. If both ECC step size and ECC strength are already set (usually by DT)
> + *    then check if it is supported by this controller.
> + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
> + * 3. Otherwise, try to match the ECC step size and ECC strength closest
> + *    to the chip's requirement. If available OOB size can't fit the chip
> + *    requirement then fallback to the maximum ECC step size and ECC strength
> + *    and print the warning.
> + *
> + * On success, the chosen ECC settings are set.
> + */
> +int nand_ecc_param_setup(struct nand_chip *chip,
> +			 const struct nand_ecc_caps *caps, int oobavail)

Can we rename this function "nand_ecc_choose_conf()".

Re: [PATCH v2 04/14] mtd: rawnand: qcom: use the ecc strength from device parameter

[Boris Brezillon]

On Thu,  3 May 2018 17:50:31 +0530
Abhishek Sahu <absahu@codeaurora.org> wrote:

> Currently the driver uses the ECC strength specified in DT.
> The QPIC/EBI2 NAND supports 4 or 8-bit ECC correction. The same
> kind of board can have different NAND parts so use the ECC
> strength from device parameters if it is not specified in DT.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
> 
>   1. Removed the custom logic and used the helper fuction.
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index b554fb6..a8d71ce 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2315,13 +2315,21 @@ static int qcom_nand_ooblayout_free(struct mtd_info *mtd, int section,
>  	.free = qcom_nand_ooblayout_free,
>  };
>  
> +static int
> +qcom_nandc_calc_ecc_bytes(int step_size, int strength)
> +{
> +	return strength == 4 ? 12 : 16;
> +}
> +NAND_ECC_CAPS_SINGLE(qcom_nandc_ecc_caps, qcom_nandc_calc_ecc_bytes,
> +		     NANDC_STEP_SIZE, 4, 8);
> +
>  static int qcom_nand_host_setup(struct qcom_nand_host *host)
>  {
>  	struct nand_chip *chip = &host->chip;
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	struct nand_ecc_ctrl *ecc = &chip->ecc;
>  	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> -	int cwperpage, bad_block_byte;
> +	int cwperpage, bad_block_byte, ret;
>  	bool wide_bus;
>  	int ecc_mode = 1;
>  
> @@ -2334,8 +2342,20 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
>  		return -EINVAL;
>  	}
>  
> +	cwperpage = mtd->writesize / ecc->size;

Looks like you're still expecting nand-ecc-step-size to be defined in
the DT, which does not really make sense since you only support one
size: NANDC_STEP_SIZE.

You should remove the

	if (ecc->size != NANDC_STEP_SIZE) {
		dev_err(nandc->dev, "invalid ecc size\n");
		return -EINVAL;
	}

block, then do:

	cwperpage = mtd->writesize / NANDC_STEP_SIZE;

and finally let the nand_ecc_param_setup() function choose the best
config for you.

> +
> +	/*
> +	 * Each CW has 4 available OOB bytes which will be protected with ECC
> +	 * so remaining bytes can be used for ECC.
> +	 */
> +	ret = nand_ecc_param_setup(chip, &qcom_nandc_ecc_caps,
> +				   mtd->oobsize - (cwperpage << 2));

Please stop doing useless optimizations like. That brings nothing and
obfuscates the code a bit more. You say in the comment that each
codeword has 4 protected OOB bytes that can be used by the upper layer,
so just do (cwperpage * 4) and let gcc optimize that for you.

> +	if (ret) {
> +		dev_err(nandc->dev, "No valid ecc settings possible\n");
> +		return ret;
> +	}
> +
>  	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
> -
>  	if (ecc->strength >= 8) {
>  		/* 8 bit ECC defaults to BCH ECC on all platforms */
>  		host->bch_enabled = true;
> @@ -2403,7 +2423,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
>  
>  	mtd_set_ooblayout(mtd, &qcom_nand_ooblayout_ops);
>  
> -	cwperpage = mtd->writesize / ecc->size;
>  	nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage,
>  				     cwperpage);
>  
> @@ -2419,12 +2438,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
>  	 * for 8 bit ECC
>  	 */
>  	host->cw_size = host->cw_data + ecc->bytes;
> -
> -	if (ecc->bytes * (mtd->writesize / ecc->size) > mtd->oobsize) {
> -		dev_err(nandc->dev, "ecc data doesn't fit in OOB area\n");
> -		return -EINVAL;
> -	}
> -
>  	bad_block_byte = mtd->writesize - host->cw_size * (cwperpage - 1) + 1;
>  
>  	host->cfg0 = (cwperpage - 1) << CW_PER_PAGE

Re: [PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional

[Boris Brezillon]

On Thu,  3 May 2018 17:50:30 +0530
Abhishek Sahu <absahu@codeaurora.org> wrote:

> Now, nand-ecc-strength is optional. If specified in DT, then
> controller will use this ECC strength otherwise ECC strength will
> be calculated according to chip requirement and available OOB size.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
> 
>   NEW PATCH
> 
>  Documentation/devicetree/bindings/mtd/qcom_nandc.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
> index 73d336be..f246aa0 100644
> --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
> +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
> @@ -45,11 +45,13 @@ Required properties:
>  			number (e.g., 0, 1, 2, etc.)
>  - #address-cells:	see partition.txt
>  - #size-cells:		see partition.txt
> -- nand-ecc-strength:	see nand.txt
>  - nand-ecc-step-size:	must be 512. see nand.txt for more details.

As mentioned in my other review, no need to specify nand-ecc-step-size
if you don't have a choice. You can remove the property and say that
nand-ecc-strength encodes the ECC strength for 512 bytes chunks.

>  
>  Optional properties:
>  - nand-bus-width:	see nand.txt
> +- nand-ecc-strength:	see nand.txt. If not specified, then ECC strength will
> +			be used according to chip requirement and available
> +			OOB size.
>  
>  Each nandcs device node may optionally contain a 'partitions' sub-node, which
>  further contains sub-nodes describing the flash partition mapping. See

Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters

[Masahiro Yamada]

2018-05-07 16:39 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Mon, 7 May 2018 12:40:39 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> 2018-05-03 21:20 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
>> > commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
>> > match, maximize ECC settings") provides generic helpers which
>> > drivers can use for setting up ECC parameters.
>> >
>> > Since same board can have different ECC strength nand chips so
>> > following is the logic for setting up ECC strength and ECC step
>> > size, which can be used by most of the drivers.
>> >
>> > 1. If both ECC step size and ECC strength are already set
>> >    (usually by DT) then just check whether this setting
>> >    is supported by NAND controller.
>> > 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
>> >    supported by NAND controller.
>> > 3. Otherwise, try to match the ECC step size and ECC strength closest
>> >    to the chip's requirement. If available OOB size can't fit the chip
>> >    requirement then select maximum ECC strength which can be fit with
>> >    available OOB size with warning.
>> >
>> > This patch introduces nand_ecc_param_setup function which calls the
>> > required helper functions for the above logic. The drivers can use
>> > this single function instead of calling the 3 helper functions
>> > individually.
>> >
>> > CC: Masahiro Yamada <yamada.masahiro@socionext.com>
>> > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> > ---
>> > * Changes from v1:
>> >
>> >   NEW PATCH
>> >
>> >  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
>> >  include/linux/mtd/rawnand.h      |  3 +++
>> >  2 files changed, 45 insertions(+)
>> >
>> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
>> > index 72f3a89..dd7a984 100644
>> > --- a/drivers/mtd/nand/raw/nand_base.c
>> > +++ b/drivers/mtd/nand/raw/nand_base.c
>> > @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
>> >  }
>> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>> >
>> > +/**
>> > + * nand_ecc_param_setup - Set the ECC strength and ECC step size
>> > + * @chip: nand chip info structure
>> > + * @caps: ECC engine caps info structure
>> > + * @oobavail: OOB size that the ECC engine can use
>> > + *
>> > + * Choose the ECC strength according to following logic
>> > + *
>> > + * 1. If both ECC step size and ECC strength are already set (usually by DT)
>> > + *    then check if it is supported by this controller.
>> > + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
>> > + * 3. Otherwise, try to match the ECC step size and ECC strength closest
>> > + *    to the chip's requirement. If available OOB size can't fit the chip
>> > + *    requirement then fallback to the maximum ECC step size and ECC strength
>> > + *    and print the warning.
>> > + *
>> > + * On success, the chosen ECC settings are set.
>> > + */
>> > +int nand_ecc_param_setup(struct nand_chip *chip,
>> > +                        const struct nand_ecc_caps *caps, int oobavail)
>> > +{
>> > +       int ret;
>> > +
>> > +       if (chip->ecc.size && chip->ecc.strength)
>> > +               return nand_check_ecc_caps(chip, caps, oobavail);
>> > +
>> > +       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>> > +               return nand_maximize_ecc(chip, caps, oobavail);
>> > +
>> > +       if (!nand_match_ecc_req(chip, caps, oobavail))
>> > +               return 0;
>> > +
>> > +       ret = nand_maximize_ecc(chip, caps, oobavail);
>>
>>
>> Why two calls for nand_maximize_ecc()?
>
> As long as the code does the same thing, I don't care that much.
>
>>
>> My code is simpler,
>
> and I don't see how your code is simpler. Mainly a matter of taste
> AFAICS.
>
>> and does not display
>> false-positive warning.
>
> I agree on the false-positive warning though, this should be avoided.
>
>>
>>
>> > +       if (!ret)
>> > +               pr_warn("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n",
>> > +                       chip->ecc_step_ds, chip->ecc_strength_ds,
>> > +                       chip->ecc.size, chip->ecc.strength);
>>
>>
>> This is annoying.
>>
>> {ecc_step_ds, ecc_strength_ds} are not provided by Non-ONFi devices.
>>
>> So,
>>   ECC (step, strength) = (0, 0) not supported on this controller.
>
> Well, if you have a chip that requires ECC but exposes 0bits/0bytes
> then this should be fixed. 0,0 should only be valid when the chip does
> not require ECC at all (so, only really old chips). For all other chips,
> including non-ONFI ones, we should have a valid value here.


Sorry, it was my misunderstanding.

My NAND chip is Toshiba.

If I remember correctly, Toshiba chips were not set
with ECC requirements in the past,
but as far as I tested the latest kernel now,
the ECC requirement was set by
drivers/mtd/nand/raw/nand_toshiba.c





>>
>> will be always displayed.
>>
>>
>> The strength will be checked by nand_ecc_strength_good() anyway.
>
> True. So, I agree that the pr_warn() is unneeded, but I still think we
> should fix all cases where ECC reqs are missing, so if you have such a
> setup, please add some code to nand_<vendor>.c to initialize
> ->ecc_xxx_ds properly.
>

If we decide to not display pr_warn(),
I think the code like denali_ecc_setup() should work, and simple.





-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters

[Abhishek Sahu]

On 2018-05-08 11:44, Masahiro Yamada wrote:
> 2018-05-07 16:39 GMT+09:00 Boris Brezillon 
> <boris.brezillon@bootlin.com>:
>> On Mon, 7 May 2018 12:40:39 +0900
>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> 
>>> 2018-05-03 21:20 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
>>> > commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
>>> > match, maximize ECC settings") provides generic helpers which
>>> > drivers can use for setting up ECC parameters.
>>> >
>>> > Since same board can have different ECC strength nand chips so
>>> > following is the logic for setting up ECC strength and ECC step
>>> > size, which can be used by most of the drivers.
>>> >
>>> > 1. If both ECC step size and ECC strength are already set
>>> >    (usually by DT) then just check whether this setting
>>> >    is supported by NAND controller.
>>> > 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
>>> >    supported by NAND controller.
>>> > 3. Otherwise, try to match the ECC step size and ECC strength closest
>>> >    to the chip's requirement. If available OOB size can't fit the chip
>>> >    requirement then select maximum ECC strength which can be fit with
>>> >    available OOB size with warning.
>>> >
>>> > This patch introduces nand_ecc_param_setup function which calls the
>>> > required helper functions for the above logic. The drivers can use
>>> > this single function instead of calling the 3 helper functions
>>> > individually.
>>> >
>>> > CC: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>>> > ---
>>> > * Changes from v1:
>>> >
>>> >   NEW PATCH
>>> >
>>> >  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
>>> >  include/linux/mtd/rawnand.h      |  3 +++
>>> >  2 files changed, 45 insertions(+)
>>> >
>>> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
>>> > index 72f3a89..dd7a984 100644
>>> > --- a/drivers/mtd/nand/raw/nand_base.c
>>> > +++ b/drivers/mtd/nand/raw/nand_base.c
>>> > @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
>>> >  }
>>> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>>> >
>>> > +/**
>>> > + * nand_ecc_param_setup - Set the ECC strength and ECC step size
>>> > + * @chip: nand chip info structure
>>> > + * @caps: ECC engine caps info structure
>>> > + * @oobavail: OOB size that the ECC engine can use
>>> > + *
>>> > + * Choose the ECC strength according to following logic
>>> > + *
>>> > + * 1. If both ECC step size and ECC strength are already set (usually by DT)
>>> > + *    then check if it is supported by this controller.
>>> > + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
>>> > + * 3. Otherwise, try to match the ECC step size and ECC strength closest
>>> > + *    to the chip's requirement. If available OOB size can't fit the chip
>>> > + *    requirement then fallback to the maximum ECC step size and ECC strength
>>> > + *    and print the warning.
>>> > + *
>>> > + * On success, the chosen ECC settings are set.
>>> > + */
>>> > +int nand_ecc_param_setup(struct nand_chip *chip,
>>> > +                        const struct nand_ecc_caps *caps, int oobavail)
>>> > +{
>>> > +       int ret;
>>> > +
>>> > +       if (chip->ecc.size && chip->ecc.strength)
>>> > +               return nand_check_ecc_caps(chip, caps, oobavail);
>>> > +
>>> > +       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>>> > +               return nand_maximize_ecc(chip, caps, oobavail);
>>> > +
>>> > +       if (!nand_match_ecc_req(chip, caps, oobavail))
>>> > +               return 0;
>>> > +
>>> > +       ret = nand_maximize_ecc(chip, caps, oobavail);
>>> 
>>> 
>>> Why two calls for nand_maximize_ecc()?
>> 
>> As long as the code does the same thing, I don't care that much.
>> 
>>> 
>>> My code is simpler,
>> 
>> and I don't see how your code is simpler. Mainly a matter of taste
>> AFAICS.
>> 
>>> and does not display
>>> false-positive warning.
>> 
>> I agree on the false-positive warning though, this should be avoided.
>> 
>>> 
>>> 
>>> > +       if (!ret)
>>> > +               pr_warn("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n",
>>> > +                       chip->ecc_step_ds, chip->ecc_strength_ds,
>>> > +                       chip->ecc.size, chip->ecc.strength);
>>> 
>>> 
>>> This is annoying.
>>> 
>>> {ecc_step_ds, ecc_strength_ds} are not provided by Non-ONFi devices.
>>> 
>>> So,
>>>   ECC (step, strength) = (0, 0) not supported on this controller.
>> 
>> Well, if you have a chip that requires ECC but exposes 0bits/0bytes
>> then this should be fixed. 0,0 should only be valid when the chip does
>> not require ECC at all (so, only really old chips). For all other 
>> chips,
>> including non-ONFI ones, we should have a valid value here.
> 
> 
> Sorry, it was my misunderstanding.
> 
> My NAND chip is Toshiba.
> 
> If I remember correctly, Toshiba chips were not set
> with ECC requirements in the past,
> but as far as I tested the latest kernel now,
> the ECC requirement was set by
> drivers/mtd/nand/raw/nand_toshiba.c
> 
> 
> 
> 
> 
>>> 
>>> will be always displayed.
>>> 
>>> 
>>> The strength will be checked by nand_ecc_strength_good() anyway.
>> 
>> True. So, I agree that the pr_warn() is unneeded, but I still think we
>> should fix all cases where ECC reqs are missing, so if you have such a
>> setup, please add some code to nand_<vendor>.c to initialize
>> ->ecc_xxx_ds properly.
>> 
> 
> If we decide to not display pr_warn(),
> I think the code like denali_ecc_setup() should work, and simple.

  Thanks Boris and Masahiro.
  I will remove this print and then we can use code like 
denali_ecc_setup.

  Regards,
  Abhishek

Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters

[Abhishek Sahu]

On 2018-05-07 13:46, Boris Brezillon wrote:
> On Thu,  3 May 2018 17:50:28 +0530
> Abhishek Sahu <absahu@codeaurora.org> wrote:
> 
>> commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
>> match, maximize ECC settings") provides generic helpers which
>> drivers can use for setting up ECC parameters.
>> 
>> Since same board can have different ECC strength nand chips so
>> following is the logic for setting up ECC strength and ECC step
>> size, which can be used by most of the drivers.
>> 
>> 1. If both ECC step size and ECC strength are already set
>>    (usually by DT) then just check whether this setting
>>    is supported by NAND controller.
>> 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
>>    supported by NAND controller.
>> 3. Otherwise, try to match the ECC step size and ECC strength closest
>>    to the chip's requirement. If available OOB size can't fit the chip
>>    requirement then select maximum ECC strength which can be fit with
>>    available OOB size with warning.
>> 
>> This patch introduces nand_ecc_param_setup function which calls the
>> required helper functions for the above logic. The drivers can use
>> this single function instead of calling the 3 helper functions
>> individually.
>> 
>> CC: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v1:
>> 
>>   NEW PATCH
>> 
>>  drivers/mtd/nand/raw/nand_base.c | 42 
>> ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/rawnand.h      |  3 +++
>>  2 files changed, 45 insertions(+)
>> 
>> diff --git a/drivers/mtd/nand/raw/nand_base.c 
>> b/drivers/mtd/nand/raw/nand_base.c
>> index 72f3a89..dd7a984 100644
>> --- a/drivers/mtd/nand/raw/nand_base.c
>> +++ b/drivers/mtd/nand/raw/nand_base.c
>> @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
>>  }
>>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>> 
>> +/**
>> + * nand_ecc_param_setup - Set the ECC strength and ECC step size
>> + * @chip: nand chip info structure
>> + * @caps: ECC engine caps info structure
>> + * @oobavail: OOB size that the ECC engine can use
>> + *
>> + * Choose the ECC strength according to following logic
>> + *
>> + * 1. If both ECC step size and ECC strength are already set (usually 
>> by DT)
>> + *    then check if it is supported by this controller.
>> + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
>> + * 3. Otherwise, try to match the ECC step size and ECC strength 
>> closest
>> + *    to the chip's requirement. If available OOB size can't fit the 
>> chip
>> + *    requirement then fallback to the maximum ECC step size and ECC 
>> strength
>> + *    and print the warning.
>> + *
>> + * On success, the chosen ECC settings are set.
>> + */
>> +int nand_ecc_param_setup(struct nand_chip *chip,
>> +			 const struct nand_ecc_caps *caps, int oobavail)
> 
> Can we rename this function "nand_ecc_choose_conf()".

  Thanks Boris.
  I will rename this function.

  Regards,
  Abhishek

Re: [PATCH v2 04/14] mtd: rawnand: qcom: use the ecc strength from device parameter

[Abhishek Sahu]

On 2018-05-07 13:58, Boris Brezillon wrote:
> On Thu,  3 May 2018 17:50:31 +0530
> Abhishek Sahu <absahu@codeaurora.org> wrote:
> 
>> Currently the driver uses the ECC strength specified in DT.
>> The QPIC/EBI2 NAND supports 4 or 8-bit ECC correction. The same
>> kind of board can have different NAND parts so use the ECC
>> strength from device parameters if it is not specified in DT.
>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v1:
>> 
>>   1. Removed the custom logic and used the helper fuction.
>> 
>>  drivers/mtd/nand/raw/qcom_nandc.c | 31 
>> ++++++++++++++++++++++---------
>>  1 file changed, 22 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
>> b/drivers/mtd/nand/raw/qcom_nandc.c
>> index b554fb6..a8d71ce 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -2315,13 +2315,21 @@ static int qcom_nand_ooblayout_free(struct 
>> mtd_info *mtd, int section,
>>  	.free = qcom_nand_ooblayout_free,
>>  };
>> 
>> +static int
>> +qcom_nandc_calc_ecc_bytes(int step_size, int strength)
>> +{
>> +	return strength == 4 ? 12 : 16;
>> +}
>> +NAND_ECC_CAPS_SINGLE(qcom_nandc_ecc_caps, qcom_nandc_calc_ecc_bytes,
>> +		     NANDC_STEP_SIZE, 4, 8);
>> +
>>  static int qcom_nand_host_setup(struct qcom_nand_host *host)
>>  {
>>  	struct nand_chip *chip = &host->chip;
>>  	struct mtd_info *mtd = nand_to_mtd(chip);
>>  	struct nand_ecc_ctrl *ecc = &chip->ecc;
>>  	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> -	int cwperpage, bad_block_byte;
>> +	int cwperpage, bad_block_byte, ret;
>>  	bool wide_bus;
>>  	int ecc_mode = 1;
>> 
>> @@ -2334,8 +2342,20 @@ static int qcom_nand_host_setup(struct 
>> qcom_nand_host *host)
>>  		return -EINVAL;
>>  	}
>> 
>> +	cwperpage = mtd->writesize / ecc->size;
> 
> Looks like you're still expecting nand-ecc-step-size to be defined in
> the DT, which does not really make sense since you only support one
> size: NANDC_STEP_SIZE.
> 
> You should remove the
> 
> 	if (ecc->size != NANDC_STEP_SIZE) {
> 		dev_err(nandc->dev, "invalid ecc size\n");
> 		return -EINVAL;
> 	}
> 
> block, then do:
> 
> 	cwperpage = mtd->writesize / NANDC_STEP_SIZE;
> 
> and finally let the nand_ecc_param_setup() function choose the best
> config for you.
> 

  Correct Boris.
  It only supports one step size so we can remove this DT
  property. I will make the changes.

>> +
>> +	/*
>> +	 * Each CW has 4 available OOB bytes which will be protected with 
>> ECC
>> +	 * so remaining bytes can be used for ECC.
>> +	 */
>> +	ret = nand_ecc_param_setup(chip, &qcom_nandc_ecc_caps,
>> +				   mtd->oobsize - (cwperpage << 2));
> 
> Please stop doing useless optimizations like. That brings nothing and
> obfuscates the code a bit more. You say in the comment that each
> codeword has 4 protected OOB bytes that can be used by the upper layer,
> so just do (cwperpage * 4) and let gcc optimize that for you.
> 

  Sure. I will change it.

  Thanks,
  Abhishek

>> +	if (ret) {
>> +		dev_err(nandc->dev, "No valid ecc settings possible\n");
>> +		return ret;
>> +	}
>> +
>>  	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
>> -
>>  	if (ecc->strength >= 8) {
>>  		/* 8 bit ECC defaults to BCH ECC on all platforms */
>>  		host->bch_enabled = true;
>> @@ -2403,7 +2423,6 @@ static int qcom_nand_host_setup(struct 
>> qcom_nand_host *host)
>> 
>>  	mtd_set_ooblayout(mtd, &qcom_nand_ooblayout_ops);
>> 
>> -	cwperpage = mtd->writesize / ecc->size;
>>  	nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage,
>>  				     cwperpage);
>> 
>> @@ -2419,12 +2438,6 @@ static int qcom_nand_host_setup(struct 
>> qcom_nand_host *host)
>>  	 * for 8 bit ECC
>>  	 */
>>  	host->cw_size = host->cw_data + ecc->bytes;
>> -
>> -	if (ecc->bytes * (mtd->writesize / ecc->size) > mtd->oobsize) {
>> -		dev_err(nandc->dev, "ecc data doesn't fit in OOB area\n");
>> -		return -EINVAL;
>> -	}
>> -
>>  	bad_block_byte = mtd->writesize - host->cw_size * (cwperpage - 1) + 
>> 1;
>> 
>>  	host->cfg0 = (cwperpage - 1) << CW_PER_PAGE

Re: [PATCH v2 02/14] mtd: rawnand: denali: use helper function for ecc setup

[Miquel Raynal]

Hi Abhishek,

On Thu,  3 May 2018 17:50:29 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> Now, the NAND base layer has helper function for ecc
> parameters setup which does the same thing.

Even if this message has a meaning in the series, I would prefer
something more generic like:

"Use the NAND core helper function xxx() to tune the ECC parameters
instead of the function locally defined."

With this change:

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional

[Miquel Raynal]

Hi Abhishek,

On Thu,  3 May 2018 17:50:30 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> Now, nand-ecc-strength is optional. If specified in DT, then
> controller will use this ECC strength otherwise ECC strength will
> be calculated according to chip requirement and available OOB size.

Same comment as before: don't start the commit log with "now,".

Thanks,
Miquèl

Re: [PATCH v2 05/14] mtd: rawnand: qcom: wait for desc completion in all BAM channels

[Miquel Raynal]

Hi Abhishek,

On Thu,  3 May 2018 17:50:32 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> The BAM has 3 channels - tx, rx and command. command channel
> is used for register read/writes, tx channel for data writes
> and rx channel for data reads. Currently, the driver assumes the
> transfer completion once it gets all the command descriptor
> completed. Sometimes, there is race condition in data channel

"Sometimes, there is a race condition between the data channel (rx/tx)
and the command channel completion. In these cases, ..."

> (tx/rx) and command channel completion and in these cases,
> the data in buffer is not valid during the small window between

           ^ present in the buffer ?

> command descriptor completion and data descriptor completion.
> 
> Now, the changes have been made to assign the callback for

It is preferable to use a descriptive tense when you expose what the
patch does. Something like "Change <this> to assign ..."

> channel's final descriptor. The DMA will generate the callback
> when all the descriptors have completed in that channel.
> The NAND transfer will be completed only when all required
> DMA channels have generated the completion callback.
> 

It looks like this is a fix that is a good candidate for stable trees,
you might want to add the relevant tags.

> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
> 
>   NONE
> 
>   1. Removed the custom logic and used the helper fuction.
>  drivers/mtd/nand/raw/qcom_nandc.c | 55 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index a8d71ce..3d1ff54 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -213,6 +213,8 @@
>  #define QPIC_PER_CW_CMD_SGL		32
>  #define QPIC_PER_CW_DATA_SGL		8
>  
> +#define QPIC_NAND_COMPLETION_TIMEOUT	msecs_to_jiffies(2000)

That's huge, but why not, it's a timeout anyway.

> +
>  /*
>   * Flags used in DMA descriptor preparation helper functions
>   * (i.e. read_reg_dma/write_reg_dma/read_data_dma/write_data_dma)
> @@ -245,6 +247,11 @@
>   * @tx_sgl_start - start index in data sgl for tx.
>   * @rx_sgl_pos - current index in data sgl for rx.
>   * @rx_sgl_start - start index in data sgl for rx.
> + * @first_chan_done - if current transfer already has got first channel
> + *		      DMA desc completion.
> + * @txn_done - completion for nand transfer.

s/nand/NAND/

> + * @last_data_desc - last DMA desc in data channel (tx/rx).
> + * @last_cmd_desc - last DMA desc in command channel.
>   */
>  struct bam_transaction {
>  	struct bam_cmd_element *bam_ce;
> @@ -258,6 +265,10 @@ struct bam_transaction {
>  	u32 tx_sgl_start;
>  	u32 rx_sgl_pos;
>  	u32 rx_sgl_start;
> +	bool first_chan_done;
> +	struct completion txn_done;
> +	struct dma_async_tx_descriptor *last_data_desc;
> +	struct dma_async_tx_descriptor *last_cmd_desc;
>  };
>  
>  /*
> @@ -504,6 +515,8 @@ static void free_bam_transaction(struct qcom_nand_controller *nandc)
>  
>  	bam_txn->data_sgl = bam_txn_buf;
>  
> +	init_completion(&bam_txn->txn_done);
> +
>  	return bam_txn;
>  }
>  
> @@ -523,11 +536,36 @@ static void clear_bam_transaction(struct qcom_nand_controller *nandc)
>  	bam_txn->tx_sgl_start = 0;
>  	bam_txn->rx_sgl_pos = 0;
>  	bam_txn->rx_sgl_start = 0;
> +	bam_txn->last_data_desc = NULL;
> +	bam_txn->first_chan_done = false;

Are you sure you don't want to reinit last_cmd_desc here?

>  
>  	sg_init_table(bam_txn->cmd_sgl, nandc->max_cwperpage *
>  		      QPIC_PER_CW_CMD_SGL);
>  	sg_init_table(bam_txn->data_sgl, nandc->max_cwperpage *
>  		      QPIC_PER_CW_DATA_SGL);
> +
> +	reinit_completion(&bam_txn->txn_done);
> +}
> +
> +/* Callback for DMA descriptor completion */
> +static void qpic_bam_dma_done(void *data)
> +{
> +	struct bam_transaction *bam_txn = data;
> +
> +	/*
> +	 * In case of data transfer with NAND, 2 callbacks will be generated.
> +	 * One for command channel and another one for data channel.
> +	 * If current transaction has data descriptors then check if its
> +	 * already got one DMA channel completion callback. In this case
> +	 * make the NAND transfer complete otherwise mark first_chan_done true
> +	 * and wait for next channel DMA completion callback.
> +	 */
> +	if (bam_txn->last_data_desc && !bam_txn->first_chan_done) {
> +		bam_txn->first_chan_done = true;
> +		return;
> +	}

There is a lot of new variables just to wait for two bam_dma_done().
Why not just creating a boolean like "wait_second completion",
initialize it in prepare_bam_async_desc to true when needed and
complete txn_done when it's false, that's all:

        if (bam_txn->wait_second_completion) {
                bam_txn->wait_second_completion = false;
                return;
        }

> +
> +	complete(&bam_txn->txn_done);
>  }
>  
>  static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip)
> @@ -756,6 +794,12 @@ static int prepare_bam_async_desc(struct qcom_nand_controller *nandc,
>  
>  	desc->dma_desc = dma_desc;
>  
> +	/* update last data/command descriptor */
> +	if (chan == nandc->cmd_chan)
> +		bam_txn->last_cmd_desc = dma_desc;
> +	else
> +		bam_txn->last_data_desc = dma_desc;
> +

Is there a reason for the "last_" prefix? why not current_*_desc or
just *_desc? (this is a real question :) ). Correct me if I'm wrong but
you have a scatter-gather list of DMA transfers that are mapped to form
one DMA descriptor, so there is no "last" descriptor, right?

Otherwise, as I told you above, why not just a:

        if (chan == nandc->data_chan)
                bam_txn->wait_second_completion = true;

>  	list_add_tail(&desc->node, &nandc->desc_list);
>  
>  	return 0;
> @@ -1273,10 +1317,19 @@ static int submit_descs(struct qcom_nand_controller *nandc)
>  		cookie = dmaengine_submit(desc->dma_desc);
>  
>  	if (nandc->props->is_bam) {
> +		bam_txn->last_cmd_desc->callback = qpic_bam_dma_done;
> +		bam_txn->last_cmd_desc->callback_param = bam_txn;
> +		if (bam_txn->last_data_desc) {
> +			bam_txn->last_data_desc->callback = qpic_bam_dma_done;
> +			bam_txn->last_data_desc->callback_param = bam_txn;
> +		}

Why don't you do this directly in prepare_bam_async_desc?

With:

        dma_desc->callback = ...
        dma_desc->callback_param = ...

> +
>  		dma_async_issue_pending(nandc->tx_chan);
>  		dma_async_issue_pending(nandc->rx_chan);
> +		dma_async_issue_pending(nandc->cmd_chan);
>  
> -		if (dma_sync_wait(nandc->cmd_chan, cookie) != DMA_COMPLETE)
> +		if (!wait_for_completion_timeout(&bam_txn->txn_done,
> +						 QPIC_NAND_COMPLETION_TIMEOUT))
>  			return -ETIMEDOUT;
>  	} else {
>  		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE)



-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 06/14] mtd: rawnand: qcom: erased page detection for uncorrectable errors only

[Miquel Raynal]

Hi Abhishek,

On Thu,  3 May 2018 17:50:33 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> Following is the flow in the HW if controller tries to read erased
> page

Nit:  ^ missing ':'

> 
> 1. First ECC uncorrectable error will be generated from ECC engine
>    since ECC engine first calculates the ECC with all 0xff and match
>    the calculated ECC with ECC code in OOB (which is again all 0xff).
> 2. After getting ECC error, erased CW detection logic will be
>    applied which is different for BCH and RS ECC
>     a. For BCH, HW checks if all the bytes in page are 0xff and then
>        it updates the status in separate register
>        NAND_ERASED_CW_DETECT_STATUS.
>     b. For RS ECC, the HW reports the same error when reading an
>        erased CW, but it notifies that it is an erased CW by
>        placing special characters at certain offsets in the
>        buffer.
> 
> So the erased CW detect status should be checked only if ECC engine
> generated the uncorrectable error.
> 
> Currently for all other operational errors also (like TIMEOUT, MPU
> errors, etc.), the erased CW detect logic is being applied so fix this
> and return EIO for other operational errors.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
> 
>   1. Added more detail in commit message
>   2. Added comment before each if/else

Thanks for that, it's much more ease to review :)

>   3. Removed redundant check for BS_UNCORRECTABLE_BIT
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 65 ++++++++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 22 deletions(-)
> 

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH v2 07/14] mtd: rawnand: qcom: fix null pointer access for erased page detection

[Miquel Raynal]

Hi Abhishek,

On Thu,  3 May 2018 17:50:34 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> parse_read_errors can be called with only oob_buf in which case
> data_buf will be NULL.  If data_buf is NULL, then don’t
> treat this page as completely erased in case of ECC uncorrectable
> error for RS ECC. For BCH ECC, the controller itself tells
> regarding erased page in status register.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
> 
>   1. Added more detail in commit message
>   2. Added comment before each if/else

Again, thanks for that.

> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>



-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 09/14] mtd: rawnand: qcom: modify write_oob to remove read codeword part

[Miquel Raynal]

Hi Abhishek,

Some nitpicking below.

On Thu,  3 May 2018 17:50:36 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> QCOM NAND layout protect available OOB data bytes with ECC also so

           ^controller

> when ecc->write_oob (qcom_nandc_write_oob) is being called then it

You can just state "->write_oob()"

> can't update just OOB bytes. Currently, it first reads the last
> codeword which includes old OOB bytes. Then it updates the old OOB
> bytes with new one and then again writes the codeword back.

                 ones?

> The reading codeword is unnecessary since all the other bytes
> should be 0xff only.

                                      since the user is responsible to
have these bytes cleared to 0xFF.

> 
> This patch removes the read part and updates the oob bytes with

s/oob/OOB/

> all other data bytes as 0xff.

The end of the sentence is not clear for me. Do you mean that padding
with 0xFF is realized before write?

> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
> 
>  NEW CHANGE
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 61d0e7d..f85d8ab 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd,
>   * implements ecc->write_oob()
>   *
>   * the NAND controller cannot write only data or only oob within a codeword,

s/oob/OOB/

Remove the trailing ','

> - * since ecc is calculated for the combined codeword. we first copy the
> - * entire contents for the last codeword(data + oob), replace the old oob
> - * with the new one in chip->oob_poi, and then write the entire codeword.
> - * this read-copy-write operation results in a slight performance loss.
> + * since ecc is calculated for the combined codeword. So make all the data

s/ecc/ECC/

> + * bytes as 0xff and update the oob from chip->oob_poi, and then write
> + * the entire codeword again.

What about "Pad the data area with OxFF before writing."?

>   */
>  static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
>  				int page)
> @@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
>  	int data_size, oob_size;
>  	int ret;
>  
> -	host->use_ecc = true;
> -
> -	clear_bam_transaction(nandc);
> -	ret = copy_last_cw(host, page);
> -	if (ret)
> -		return ret;
> -
> -	clear_read_regs(nandc);
>  	clear_bam_transaction(nandc);
>  
>  	/* calculate the data and oob size for the last codeword/step */
>  	data_size = ecc->size - ((ecc->steps - 1) << 2);
>  	oob_size = mtd->oobavail;
> +	host->use_ecc = true;

You don't need to move this line here, do you?

>  
> +	memset(nandc->data_buffer, 0xff, host->cw_data);
>  	/* override new oob content to last codeword */
>  	mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, oob,
>  				    0, mtd->oobavail);


Once fixed, you can add my:

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH v2 10/14] mtd: rawnand: qcom: fix return value for raw page read

[Miquel Raynal]

Hi Abhishek,

On Thu,  3 May 2018 17:50:37 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> Currently zero is being returned for all raw page read so
> fix the same.

What about "Fix value returned by ->read_page_raw() to be the
actual operation status, instead of always 0."?

> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v1:
> 
>  NEW CHANGE
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index f85d8ab..17b7f3af 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -1900,7 +1900,7 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,
>  
>  	free_descs(nandc);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /* implements ecc->read_oob() */

Thanks,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 05/14] mtd: rawnand: qcom: wait for desc completion in all BAM channels

[Abhishek Sahu]

On 2018-05-22 12:17, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Thu,  3 May 2018 17:50:32 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> The BAM has 3 channels - tx, rx and command. command channel
>> is used for register read/writes, tx channel for data writes
>> and rx channel for data reads. Currently, the driver assumes the
>> transfer completion once it gets all the command descriptor
>> completed. Sometimes, there is race condition in data channel
> 
> "Sometimes, there is a race condition between the data channel (rx/tx)
> and the command channel completion. In these cases, ..."
> 
>> (tx/rx) and command channel completion and in these cases,
>> the data in buffer is not valid during the small window between
> 
>            ^ present in the buffer ?
> 
>> command descriptor completion and data descriptor completion.
>> 
>> Now, the changes have been made to assign the callback for
> 
> It is preferable to use a descriptive tense when you expose what the
> patch does. Something like "Change <this> to assign ..."
> 

  Thanks Miquel for your review.
  I will change the sentence.

>> channel's final descriptor. The DMA will generate the callback
>> when all the descriptors have completed in that channel.
>> The NAND transfer will be completed only when all required
>> DMA channels have generated the completion callback.
>> 
> 
> It looks like this is a fix that is a good candidate for stable trees,
> you might want to add the relevant tags.

  Sure. I will add the relevant tags.

> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v1:
>> 
>>   NONE
>> 
>>   1. Removed the custom logic and used the helper fuction.
>>  drivers/mtd/nand/raw/qcom_nandc.c | 55 
>> ++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 54 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
>> b/drivers/mtd/nand/raw/qcom_nandc.c
>> index a8d71ce..3d1ff54 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -213,6 +213,8 @@
>>  #define QPIC_PER_CW_CMD_SGL		32
>>  #define QPIC_PER_CW_DATA_SGL		8
>> 
>> +#define QPIC_NAND_COMPLETION_TIMEOUT	msecs_to_jiffies(2000)
> 
> That's huge, but why not, it's a timeout anyway.
> 

   Correct. This timeout will never happen in normal case.
   It will hit if something bad happened in the board.

>> +
>>  /*
>>   * Flags used in DMA descriptor preparation helper functions
>>   * (i.e. read_reg_dma/write_reg_dma/read_data_dma/write_data_dma)
>> @@ -245,6 +247,11 @@
>>   * @tx_sgl_start - start index in data sgl for tx.
>>   * @rx_sgl_pos - current index in data sgl for rx.
>>   * @rx_sgl_start - start index in data sgl for rx.
>> + * @first_chan_done - if current transfer already has got first 
>> channel
>> + *		      DMA desc completion.
>> + * @txn_done - completion for nand transfer.
> 
> s/nand/NAND/
> 
>> + * @last_data_desc - last DMA desc in data channel (tx/rx).
>> + * @last_cmd_desc - last DMA desc in command channel.
>>   */
>>  struct bam_transaction {
>>  	struct bam_cmd_element *bam_ce;
>> @@ -258,6 +265,10 @@ struct bam_transaction {
>>  	u32 tx_sgl_start;
>>  	u32 rx_sgl_pos;
>>  	u32 rx_sgl_start;
>> +	bool first_chan_done;
>> +	struct completion txn_done;
>> +	struct dma_async_tx_descriptor *last_data_desc;
>> +	struct dma_async_tx_descriptor *last_cmd_desc;
>>  };
>> 
>>  /*
>> @@ -504,6 +515,8 @@ static void free_bam_transaction(struct 
>> qcom_nand_controller *nandc)
>> 
>>  	bam_txn->data_sgl = bam_txn_buf;
>> 
>> +	init_completion(&bam_txn->txn_done);
>> +
>>  	return bam_txn;
>>  }
>> 
>> @@ -523,11 +536,36 @@ static void clear_bam_transaction(struct 
>> qcom_nand_controller *nandc)
>>  	bam_txn->tx_sgl_start = 0;
>>  	bam_txn->rx_sgl_pos = 0;
>>  	bam_txn->rx_sgl_start = 0;
>> +	bam_txn->last_data_desc = NULL;
>> +	bam_txn->first_chan_done = false;
> 
> Are you sure you don't want to reinit last_cmd_desc here?

  Each NAND data transfer will definitely have at least one command
  desc so that reinit is redundant.

  But some of the NAND transfers can have only command descriptors
  (i.e. no data desc) so, we need to reinit last_data_desc.

> 
>> 
>>  	sg_init_table(bam_txn->cmd_sgl, nandc->max_cwperpage *
>>  		      QPIC_PER_CW_CMD_SGL);
>>  	sg_init_table(bam_txn->data_sgl, nandc->max_cwperpage *
>>  		      QPIC_PER_CW_DATA_SGL);
>> +
>> +	reinit_completion(&bam_txn->txn_done);
>> +}
>> +
>> +/* Callback for DMA descriptor completion */
>> +static void qpic_bam_dma_done(void *data)
>> +{
>> +	struct bam_transaction *bam_txn = data;
>> +
>> +	/*
>> +	 * In case of data transfer with NAND, 2 callbacks will be 
>> generated.
>> +	 * One for command channel and another one for data channel.
>> +	 * If current transaction has data descriptors then check if its
>> +	 * already got one DMA channel completion callback. In this case
>> +	 * make the NAND transfer complete otherwise mark first_chan_done 
>> true
>> +	 * and wait for next channel DMA completion callback.
>> +	 */
>> +	if (bam_txn->last_data_desc && !bam_txn->first_chan_done) {
>> +		bam_txn->first_chan_done = true;
>> +		return;
>> +	}
> 
> There is a lot of new variables just to wait for two bam_dma_done().
> Why not just creating a boolean like "wait_second completion",
> initialize it in prepare_bam_async_desc to true when needed and
> complete txn_done when it's false, that's all:
> 
>         if (bam_txn->wait_second_completion) {
>                 bam_txn->wait_second_completion = false;
>                 return;
>         }
> 
>> +
>> +	complete(&bam_txn->txn_done);
>>  }
>> 
>>  static inline struct qcom_nand_host *to_qcom_nand_host(struct 
>> nand_chip *chip)
>> @@ -756,6 +794,12 @@ static int prepare_bam_async_desc(struct 
>> qcom_nand_controller *nandc,
>> 
>>  	desc->dma_desc = dma_desc;
>> 
>> +	/* update last data/command descriptor */
>> +	if (chan == nandc->cmd_chan)
>> +		bam_txn->last_cmd_desc = dma_desc;
>> +	else
>> +		bam_txn->last_data_desc = dma_desc;
>> +
> 
> Is there a reason for the "last_" prefix? why not current_*_desc or
> just *_desc? (this is a real question :) ). Correct me if I'm wrong but
> you have a scatter-gather list of DMA transfers that are mapped to form
> one DMA descriptor, so there is no "last" descriptor, right?
> 

  We have 3 DMA channels (tx/rx and command) and each channel has 
multiple
  DMA descriptors. The callback needs to be set for last
  descriptor only for that channel.

> Otherwise, as I told you above, why not just a:
> 
>         if (chan == nandc->data_chan)
>                 bam_txn->wait_second_completion = true;
> 

  This is nice idea.
  I will change the implementation accordingly.

>>  	list_add_tail(&desc->node, &nandc->desc_list);
>> 
>>  	return 0;
>> @@ -1273,10 +1317,19 @@ static int submit_descs(struct 
>> qcom_nand_controller *nandc)
>>  		cookie = dmaengine_submit(desc->dma_desc);
>> 
>>  	if (nandc->props->is_bam) {
>> +		bam_txn->last_cmd_desc->callback = qpic_bam_dma_done;
>> +		bam_txn->last_cmd_desc->callback_param = bam_txn;
>> +		if (bam_txn->last_data_desc) {
>> +			bam_txn->last_data_desc->callback = qpic_bam_dma_done;
>> +			bam_txn->last_data_desc->callback_param = bam_txn;
>> +		}
> 
> Why don't you do this directly in prepare_bam_async_desc?
> 
> With:
> 
>         dma_desc->callback = ...
>         dma_desc->callback_param = ...
> 

  prepare_bam_async_desc can be called multiple times since
  each channel can have list of DMA descriptors. We want
  to set callback only for last DMA descriptor in that
  channel.

  CMD desc1 -> Data desc1 -> Data desc2-> CMD desc2 -> CMD desc3

  In the above case, the callback should be set for
  Data desc2 and CMD desc3.

  Thanks,
  Abhishek

>> +
>>  		dma_async_issue_pending(nandc->tx_chan);
>>  		dma_async_issue_pending(nandc->rx_chan);
>> +		dma_async_issue_pending(nandc->cmd_chan);
>> 
>> -		if (dma_sync_wait(nandc->cmd_chan, cookie) != DMA_COMPLETE)
>> +		if (!wait_for_completion_timeout(&bam_txn->txn_done,
>> +						 QPIC_NAND_COMPLETION_TIMEOUT))
>>  			return -ETIMEDOUT;
>>  	} else {
>>  		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE)

Re: [PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional

[Abhishek Sahu]

On 2018-05-21 20:02, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Thu,  3 May 2018 17:50:30 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> Now, nand-ecc-strength is optional. If specified in DT, then
>> controller will use this ECC strength otherwise ECC strength will
>> be calculated according to chip requirement and available OOB size.
> 
> Same comment as before: don't start the commit log with "now,".
> 
> Thanks,
> Miquèl

  Thanks Miquel.
  I will change the commit message.

  Regards,
  Abhishek

Re: [PATCH v2 02/14] mtd: rawnand: denali: use helper function for ecc setup

[Abhishek Sahu]

On 2018-05-21 20:00, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Thu,  3 May 2018 17:50:29 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> Now, the NAND base layer has helper function for ecc
>> parameters setup which does the same thing.
> 
> Even if this message has a meaning in the series, I would prefer
> something more generic like:
> 
> "Use the NAND core helper function xxx() to tune the ECC parameters
> instead of the function locally defined."
> 

  Sure Miquel. I will change the commit message.

  Thanks,
  Abhishek

> With this change:
> 
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Thanks,
> Miquèl

Re: [PATCH v2 06/14] mtd: rawnand: qcom: erased page detection for uncorrectable errors only

[Abhishek Sahu]

On 2018-05-22 12:34, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Thu,  3 May 2018 17:50:33 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> Following is the flow in the HW if controller tries to read erased
>> page
> 
> Nit:  ^ missing ':'
> 

  Sure. I will fix this.

>> 
>> 1. First ECC uncorrectable error will be generated from ECC engine
>>    since ECC engine first calculates the ECC with all 0xff and match
>>    the calculated ECC with ECC code in OOB (which is again all 0xff).
>> 2. After getting ECC error, erased CW detection logic will be
>>    applied which is different for BCH and RS ECC
>>     a. For BCH, HW checks if all the bytes in page are 0xff and then
>>        it updates the status in separate register
>>        NAND_ERASED_CW_DETECT_STATUS.
>>     b. For RS ECC, the HW reports the same error when reading an
>>        erased CW, but it notifies that it is an erased CW by
>>        placing special characters at certain offsets in the
>>        buffer.
>> 
>> So the erased CW detect status should be checked only if ECC engine
>> generated the uncorrectable error.
>> 
>> Currently for all other operational errors also (like TIMEOUT, MPU
>> errors, etc.), the erased CW detect logic is being applied so fix this
>> and return EIO for other operational errors.
>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v1:
>> 
>>   1. Added more detail in commit message
>>   2. Added comment before each if/else
> 
> Thanks for that, it's much more ease to review :)

  Thanks Miquel for your review.

  Regards,
  Abhishek

> 
>>   3. Removed redundant check for BS_UNCORRECTABLE_BIT
>> 
>>  drivers/mtd/nand/raw/qcom_nandc.c | 65 
>> ++++++++++++++++++++++++++-------------
>>  1 file changed, 43 insertions(+), 22 deletions(-)
>> 
> 
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Thanks,
> Miquèl

Re: [PATCH v2 07/14] mtd: rawnand: qcom: fix null pointer access for erased page detection

[Abhishek Sahu]

On 2018-05-22 12:46, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Thu,  3 May 2018 17:50:34 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> parse_read_errors can be called with only oob_buf in which case
>> data_buf will be NULL.  If data_buf is NULL, then don’t
>> treat this page as completely erased in case of ECC uncorrectable
>> error for RS ECC. For BCH ECC, the controller itself tells
>> regarding erased page in status register.
>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v1:
>> 
>>   1. Added more detail in commit message
>>   2. Added comment before each if/else
> 
> Again, thanks for that.
> 
>> 
>>  drivers/mtd/nand/raw/qcom_nandc.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

  Thanks Miquel for your review.

  Regards,
  Abhishek

Re: [PATCH v2 09/14] mtd: rawnand: qcom: modify write_oob to remove read codeword part

[Abhishek Sahu]

On 2018-05-22 15:32, Miquel Raynal wrote:
> Hi Abhishek,
> 
> Some nitpicking below.
> 
> On Thu,  3 May 2018 17:50:36 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> QCOM NAND layout protect available OOB data bytes with ECC also so
> 
>            ^controller
> 
>> when ecc->write_oob (qcom_nandc_write_oob) is being called then it
> 
> You can just state "->write_oob()"
> 
>> can't update just OOB bytes. Currently, it first reads the last
>> codeword which includes old OOB bytes. Then it updates the old OOB
>> bytes with new one and then again writes the codeword back.
> 
>                  ones?
> 
>> The reading codeword is unnecessary since all the other bytes
>> should be 0xff only.
> 
>                                       since the user is responsible to
> have these bytes cleared to 0xFF.
> 
>> 
>> This patch removes the read part and updates the oob bytes with
> 
> s/oob/OOB/
> 
>> all other data bytes as 0xff.
> 
> The end of the sentence is not clear for me. Do you mean that padding
> with 0xFF is realized before write?
> 
>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v1:
>> 
>>  NEW CHANGE
>> 
>>  drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------
>>  1 file changed, 5 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
>> b/drivers/mtd/nand/raw/qcom_nandc.c
>> index 61d0e7d..f85d8ab 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(struct 
>> mtd_info *mtd,
>>   * implements ecc->write_oob()
>>   *
>>   * the NAND controller cannot write only data or only oob within a 
>> codeword,
> 
> s/oob/OOB/
> 
> Remove the trailing ','
> 
>> - * since ecc is calculated for the combined codeword. we first copy 
>> the
>> - * entire contents for the last codeword(data + oob), replace the old 
>> oob
>> - * with the new one in chip->oob_poi, and then write the entire 
>> codeword.
>> - * this read-copy-write operation results in a slight performance 
>> loss.
>> + * since ecc is calculated for the combined codeword. So make all the 
>> data
> 
> s/ecc/ECC/
> 
>> + * bytes as 0xff and update the oob from chip->oob_poi, and then 
>> write
>> + * the entire codeword again.
> 
> What about "Pad the data area with OxFF before writing."?
> 
>>   */
>>  static int qcom_nandc_write_oob(struct mtd_info *mtd, struct 
>> nand_chip *chip,
>>  				int page)
>> @@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct 
>> mtd_info *mtd, struct nand_chip *chip,
>>  	int data_size, oob_size;
>>  	int ret;
>> 
>> -	host->use_ecc = true;
>> -
>> -	clear_bam_transaction(nandc);
>> -	ret = copy_last_cw(host, page);
>> -	if (ret)
>> -		return ret;
>> -
>> -	clear_read_regs(nandc);
>>  	clear_bam_transaction(nandc);
>> 
>>  	/* calculate the data and oob size for the last codeword/step */
>>  	data_size = ecc->size - ((ecc->steps - 1) << 2);
>>  	oob_size = mtd->oobavail;
>> +	host->use_ecc = true;
> 
> You don't need to move this line here, do you?
> 
>> 
>> +	memset(nandc->data_buffer, 0xff, host->cw_data);
>>  	/* override new oob content to last codeword */
>>  	mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, 
>> oob,
>>  				    0, mtd->oobavail);
> 
> 
> Once fixed, you can add my:

  Thanks Miquel.
  I will fix all these and update the patch.

  Regards,
  Abhishek

> 
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Thanks,
> Miquèl

Re: [PATCH v2 10/14] mtd: rawnand: qcom: fix return value for raw page read

[Abhishek Sahu]

On 2018-05-22 17:34, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Thu,  3 May 2018 17:50:37 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> Currently zero is being returned for all raw page read so
>> fix the same.
> 
> What about "Fix value returned by ->read_page_raw() to be the
> actual operation status, instead of always 0."?
> 

  Sure Miquel. It looks better.
  I will change this.

  Thanks,
  Abhishek

>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v1:
>> 
>>  NEW CHANGE
>> 
>>  drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
>> b/drivers/mtd/nand/raw/qcom_nandc.c
>> index f85d8ab..17b7f3af 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -1900,7 +1900,7 @@ static int qcom_nandc_read_page_raw(struct 
>> mtd_info *mtd,
>> 
>>  	free_descs(nandc);
>> 
>> -	return 0;
>> +	return ret;
>>  }
>> 
>>  /* implements ecc->read_oob() */
> 
> Thanks,
> Miquèl