[PATCH v3 00/16] Update for QCOM NAND driver

[PATCH v3 00/16] Update for QCOM NAND driver

[Abhishek Sahu]

* v3:

1. Addressed all review comments in v2.
2. Added patch for removing redundant nand-ecc-step-size DT property.
3. Renamed ECC configuration setup function with minor code changes.
4. Modified comments and commit message for few patches.

* 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 (16):
  mtd: rawnand: helper function for setting up ECC configuration
  mtd: rawnand: denali: use helper function for ecc setup
  dt-bindings: qcom_nandc: make nand-ecc-strength optional
  dt-bindings: qcom_nandc: remove nand-ecc-step-size
  mtd: rawnand: qcom: remove dt property nand-ecc-step-size
  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         |   7 +-
 drivers/mtd/nand/raw/denali.c                      |  30 +-
 drivers/mtd/nand/raw/nand_base.c                   |  31 +
 drivers/mtd/nand/raw/qcom_nandc.c                  | 628 ++++++++++++++-------
 include/linux/mtd/rawnand.h                        |   3 +
 5 files changed, 448 insertions(+), 251 deletions(-)

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

[PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[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.

This patch introduces nand_ecc_choose_conf 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 v2:

  1. Renamed function to nand_ecc_choose_conf.
  2. Minor code reorganization to remove warning and 2 function calls
     for nand_maximize_ecc.

* Changes from v1:
  NEW PATCH

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

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..e52019d 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
 }
 EXPORT_SYMBOL_GPL(nand_maximize_ecc);
 
+/**
+ * nand_ecc_choose_conf - 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 configuration 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.
+ *
+ * On success, the chosen ECC settings are set.
+ */
+int nand_ecc_choose_conf(struct nand_chip *chip,
+			 const struct nand_ecc_caps *caps, int oobavail)
+{
+	if (chip->ecc.size && chip->ecc.strength)
+		return nand_check_ecc_caps(chip, caps, oobavail);
+
+	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
+	    !nand_match_ecc_req(chip, caps, oobavail))
+		return 0;
+
+	return nand_maximize_ecc(chip, caps, oobavail);
+}
+EXPORT_SYMBOL_GPL(nand_ecc_choose_conf);
+
 /*
  * 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..89889fa 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_choose_conf(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 v3 02/16] mtd: rawnand: denali: use helper function for ecc setup

[Abhishek Sahu]

Use the NAND core helper function nand_ecc_choose_conf to tune
the ECC parameters instead of the function locally defined.

CC: Masahiro Yamada <yamada.masahiro@socionext.com>
Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v2:

  1. Changed commit message

* 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..a586a1d 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_choose_conf(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 v3 03/16] dt-bindings: qcom_nandc: make nand-ecc-strength optional

[Abhishek Sahu]

If nand-ecc-strength 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 v2:
  NONE

* 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 v3 04/16] dt-bindings: qcom_nandc: remove nand-ecc-step-size

[Abhishek Sahu]

QCOM NAND controller supports only one step size (512) but
nand-ecc-step-size is required property in DT. This DT property
can be removed and ecc step size can be assigned in driver with
512 value.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---

Currently there is no user in mainline linux kernel for
QPIC. Following patches for this node is in review

https://patchwork.kernel.org/patch/10426405/
https://patchwork.kernel.org/patch/10426385/

If these changes got merged then I will submit another change
to remove the nand-ecc-step-size from actual DTS.

* Changes from v2:

 NEW CHANGE

 Documentation/devicetree/bindings/mtd/qcom_nandc.txt | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
index f246aa0..1123cc6 100644
--- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
+++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
@@ -45,7 +45,6 @@ Required properties:
 			number (e.g., 0, 1, 2, etc.)
 - #address-cells:	see partition.txt
 - #size-cells:		see partition.txt
-- nand-ecc-step-size:	must be 512. see nand.txt for more details.
 
 Optional properties:
 - nand-bus-width:	see nand.txt
@@ -79,7 +78,6 @@ nand-controller@1ac00000 {
 		reg = <0>;
 
 		nand-ecc-strength = <4>;
-		nand-ecc-step-size = <512>;
 		nand-bus-width = <8>;
 
 		partitions {
@@ -119,7 +117,6 @@ nand-controller@79b0000 {
 	nand@0 {
 		reg = <0>;
 		nand-ecc-strength = <4>;
-		nand-ecc-step-size = <512>;
 		nand-bus-width = <8>;
 
 		partitions {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 05/16] mtd: rawnand: qcom: remove dt property nand-ecc-step-size

[Abhishek Sahu]

QCOM NAND controller supports only one step size (512) so
nand-ecc-step-size DT property is redundant. This property
can be removed and ecc step size can be assigned with 512 value.

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

 NEW CHANGE

  1. Removed the custom logic and used the helper fuction.
 drivers/mtd/nand/raw/qcom_nandc.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index b554fb6..b538390 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2325,15 +2325,8 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
 	bool wide_bus;
 	int ecc_mode = 1;
 
-	/*
-	 * the controller requires each step consists of 512 bytes of data.
-	 * bail out if DT has populated a wrong step size.
-	 */
-	if (ecc->size != NANDC_STEP_SIZE) {
-		dev_err(nandc->dev, "invalid ecc size\n");
-		return -EINVAL;
-	}
-
+	/* controller only supports 512 bytes of data in each step */
+	ecc->size = NANDC_STEP_SIZE;
 	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
 
 	if (ecc->strength >= 8) {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 06/16] 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 v2:
  NONE

* Changes from v1:

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

 drivers/mtd/nand/raw/qcom_nandc.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index b538390..7377923 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2315,19 +2315,39 @@ 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;
 
 	/* controller only supports 512 bytes of data in each step */
 	ecc->size = NANDC_STEP_SIZE;
 	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
+	cwperpage = mtd->writesize / NANDC_STEP_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_choose_conf(chip, &qcom_nandc_ecc_caps,
+				   mtd->oobsize - cwperpage * 4);
+	if (ret) {
+		dev_err(nandc->dev, "No valid ECC settings possible\n");
+		return ret;
+	}
 
 	if (ecc->strength >= 8) {
 		/* 8 bit ECC defaults to BCH ECC on all platforms */
@@ -2396,7 +2416,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);
 
@@ -2412,12 +2431,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 v3 07/16] 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 descriptors
completed. Sometimes, there is race condition between data channel
(tx/rx) and command channel completion. In these cases,
the data present in buffer is not valid during small window
between command descriptor completion and data descriptor
completion.

This patch generates NAND transfer completion when both
(Data and Command) DMA channels have completed all its DMA
descriptors. It assigns completion callback in last
DMA descriptors of that channel and wait for completion.

Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
Cc: stable@vger.kernel.org
Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v2:
  1. Changed commit message and comments slightly
  2. Renamed wait_second_completion from first_chan_done and set
     it before submit desc
  3. Mark for stable tree

* Changes from v1:
  NONE

 drivers/mtd/nand/raw/qcom_nandc.c | 53 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 7377923..7f85ef8 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.
+ * @wait_second_completion - wait for second DMA desc completion before making
+ *			     the NAND transfer 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 wait_second_completion;
+	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,33 @@ 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->wait_second_completion = 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
+	 * (i.e. wait_second_completion is true), then set this to false
+	 * and wait for second DMA descriptor completion.
+	 */
+	if (bam_txn->wait_second_completion)
+		bam_txn->wait_second_completion = false;
+	else
+		complete(&bam_txn->txn_done);
 }
 
 static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip)
@@ -756,6 +791,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 +1314,20 @@ 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;
+			bam_txn->wait_second_completion = true;
+		}
+
 		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 v3 08/16] 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.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
* Changes from v2:
  1. Changed commit message slightly

* 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 7f85ef8..9a11827 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1576,6 +1576,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;
@@ -1597,8 +1598,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) {
@@ -1616,29 +1627,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;
 
@@ -1652,6 +1670,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 v3 09/16] 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.

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

* 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 9a11827..aab738f 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1611,13 +1611,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) {
@@ -1665,7 +1676,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 v3 10/16] 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.
This patch moves the error checking code inside read_page_ecc
so caller does not have to check explicitly for read errors.

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

* 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 aab738f..e4b87b7 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1698,6 +1698,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);
@@ -1758,12 +1759,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);
 }
 
 /*
@@ -1808,20 +1811,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() */
@@ -1911,7 +1908,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);
@@ -1920,11 +1916,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 v3 11/16] mtd: rawnand: qcom: modify write_oob to remove read codeword part

[Abhishek Sahu]

QCOM NAND controller layout protects available OOB data bytes with
ECC also so when ecc->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 ones and then again writes the codeword back.
The reading codeword is unnecessary since user is responsible to
have these bytes cleared to 0xFF.

This patch removes the read part and updates the OOB bytes with
data area padded with OxFF.

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

  1. Changed commit message and comments slightly
  2. Changed host->use_ecc assignment place

* Changes from v1:
  NEW CHANGE

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

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index e4b87b7..e02d752 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2064,11 +2064,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.
+ * the NAND controller cannot write only data or only OOB within a codeword
+ * since ECC is calculated for the combined codeword. So update the OOB from
+ * chip->oob_poi, and pad the data area with OxFF before writing.
  */
 static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
 				int page)
@@ -2081,19 +2079,13 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
 	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;
 
+	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 v3 12/16] mtd: rawnand: qcom: fix return value for raw page read

[Abhishek Sahu]

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 v2:

  1. Changed commit message

* 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 e02d752..d693b5f 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1898,7 +1898,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 v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

The QCOM NAND controller 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 HW buffer to actual 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 v2:
  1. Changed commit message and comments slightly

* 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 d693b5f..f72bc8a 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1769,41 +1769,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)
@@ -2118,6 +2083,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;
 
@@ -2128,11 +2094,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]);
 
@@ -2141,12 +2127,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 v3 14/16] 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 v2:
  NONE

* 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 f72bc8a..87f900e 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1096,7 +1096,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,
@@ -1105,19 +1106,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);
 }
 
 /*
@@ -1198,7 +1205,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);
@@ -1563,6 +1570,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.
@@ -1729,7 +1753,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,
@@ -1839,7 +1863,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;
@@ -1858,12 +1882,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() */
@@ -2082,7 +2107,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;
@@ -2109,7 +2133,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);
 
@@ -2120,9 +2144,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 v3 15/16] 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 v2:
  NONE

* 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 87f900e..34143a4 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1588,6 +1588,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 actual 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.
  */
@@ -1815,80 +1936,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 v3 16/16] 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, QCOM 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 cases, 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 v2:
  NONE

* 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 34143a4..1944b4b 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1709,20 +1709,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);
@@ -1752,10 +1835,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.
@@ -1776,31 +1855,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
@@ -1830,7 +1886,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);
 }
 
 /*
@@ -1838,7 +1899,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);
@@ -1911,7 +1972,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() */
@@ -1928,7 +1989,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() */
@@ -1955,7 +2016,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 v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:29 +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.
> 
> This patch introduces nand_ecc_choose_conf 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 v2:
> 
>   1. Renamed function to nand_ecc_choose_conf.
>   2. Minor code reorganization to remove warning and 2 function calls
>      for nand_maximize_ecc.
> 
> * Changes from v1:
>   NEW PATCH
> 
>  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
>  include/linux/mtd/rawnand.h      |  3 +++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..e52019d 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
>  }
>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>  
> +/**
> + * nand_ecc_choose_conf - 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 configuration 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.
> + *
> + * On success, the chosen ECC settings are set.
> + */
> +int nand_ecc_choose_conf(struct nand_chip *chip,
> +			 const struct nand_ecc_caps *caps, int oobavail)
> +{
> +	if (chip->ecc.size && chip->ecc.strength)
> +		return nand_check_ecc_caps(chip, caps, oobavail);
> +
> +	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
> +	    !nand_match_ecc_req(chip, caps, oobavail))
> +		return 0;
> +
> +	return nand_maximize_ecc(chip, caps, oobavail);

I personally don't mind if nand_maximize_ecc() is called twice in
the function if it clarifies the logic. Maybe the following will be
more clear for the user?

	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;

	return nand_maximize_ecc(chip, caps, oobavail);

Also, I'm not sure we should just error out when nand_check_ecc_caps()
fails. What about something more robust, like:

	int ret;

	if (chip->ecc.size && chip->ecc.strength) {
		ret = nand_check_ecc_caps(chip, caps, oobavail);
		if (ret)
			goto maximize_ecc;

		return 0;
	}

	if (chip->ecc.options & NAND_ECC_MAXIMIZE)
		goto maximize_ecc;

	ret = nand_match_ecc_req(chip, caps, oobavail);
	if (ret)
		goto maximize_ecc;

	return 0;

maximize_ecc:
	return nand_maximize_ecc(chip, caps, oobavail);

> +}
> +EXPORT_SYMBOL_GPL(nand_ecc_choose_conf);
> +
>  /*
>   * 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..89889fa 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_choose_conf(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);
>  

Thanks,
Miquèl

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

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:31 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> If nand-ecc-strength 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 v2:
>   NONE
> 
> * 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.

I think you can squash the two dt-bindings commits as they are tightly
related to each other.

>  
>  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



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

Re: [PATCH v3 05/16] mtd: rawnand: qcom: remove dt property nand-ecc-step-size

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:33 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> QCOM NAND controller supports only one step size (512) so
> nand-ecc-step-size DT property is redundant. This property
> can be removed and ecc step size can be assigned with 512 value.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v2:
> 
>  NEW CHANGE
> 
>   1. Removed the custom logic and used the helper fuction.
>  drivers/mtd/nand/raw/qcom_nandc.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index b554fb6..b538390 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2325,15 +2325,8 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
>  	bool wide_bus;
>  	int ecc_mode = 1;
>  
> -	/*
> -	 * the controller requires each step consists of 512 bytes of data.
> -	 * bail out if DT has populated a wrong step size.
> -	 */
> -	if (ecc->size != NANDC_STEP_SIZE) {
> -		dev_err(nandc->dev, "invalid ecc size\n");
> -		return -EINVAL;
> -	}
> -
> +	/* controller only supports 512 bytes of data in each step */

"512 bytes data steps"

> +	ecc->size = NANDC_STEP_SIZE;
>  	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
>  
>  	if (ecc->strength >= 8) {

Once corrected:

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

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

Re: [PATCH v3 05/16] mtd: rawnand: qcom: remove dt property nand-ecc-step-size

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:33 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> QCOM NAND controller supports only one step size (512) so
> nand-ecc-step-size DT property is redundant. This property
> can be removed and ecc step size can be assigned with 512 value.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v2:
> 
>  NEW CHANGE
> 
>   1. Removed the custom logic and used the helper fuction.
>  drivers/mtd/nand/raw/qcom_nandc.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index b554fb6..b538390 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2325,15 +2325,8 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
>  	bool wide_bus;
>  	int ecc_mode = 1;
>  
> -	/*
> -	 * the controller requires each step consists of 512 bytes of data.
> -	 * bail out if DT has populated a wrong step size.
> -	 */
> -	if (ecc->size != NANDC_STEP_SIZE) {
> -		dev_err(nandc->dev, "invalid ecc size\n");
> -		return -EINVAL;
> -	}
> -
> +	/* controller only supports 512 bytes of data in each step */

"512 bytes data steps"

> +	ecc->size = NANDC_STEP_SIZE;
>  	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
>  
>  	if (ecc->strength >= 8) {

Once corrected:

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

Re: [PATCH v3 06/16] mtd: rawnand: qcom: use the ecc strength from device parameter

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:34 +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 v2:
>   NONE

Yes you did change things:

- s/<< 2/* 4/
- updated the cwperpage location
- the block handling the ecc-step-size property has been removed in a
  previous patch

Please be careful with that, it is time consuming to review the patches
all over again.

> 
> * Changes from v1:
> 
>   1. Removed the custom logic and used the helper fuction.
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index b538390..7377923 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2315,19 +2315,39 @@ 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;
>  
>  	/* controller only supports 512 bytes of data in each step */
>  	ecc->size = NANDC_STEP_SIZE;
>  	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
> +	cwperpage = mtd->writesize / NANDC_STEP_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_choose_conf(chip, &qcom_nandc_ecc_caps,
> +				   mtd->oobsize - cwperpage * 4);

Nitpick: could you add parenthesis around (cwperpage * 4) just for
clarity.

> +	if (ret) {
> +		dev_err(nandc->dev, "No valid ECC settings possible\n");
> +		return ret;
> +	}
>  
>  	if (ecc->strength >= 8) {
>  		/* 8 bit ECC defaults to BCH ECC on all platforms */
> @@ -2396,7 +2416,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);
>  
> @@ -2412,12 +2431,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

Once corrected:

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

Thanks,
Miquèl

Re: [PATCH v3 07/16] mtd: rawnand: qcom: wait for desc completion in all BAM channels

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:35 +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 descriptors
> completed. Sometimes, there is race condition between data channel
> (tx/rx) and command channel completion. In these cases,
> the data present in buffer is not valid during small window
> between command descriptor completion and data descriptor
> completion.
> 
> This patch generates NAND transfer completion when both
> (Data and Command) DMA channels have completed all its DMA
> descriptors. It assigns completion callback in last
> DMA descriptors of that channel and wait for completion.
> 
> Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v2:
>   1. Changed commit message and comments slightly
>   2. Renamed wait_second_completion from first_chan_done and set
>      it before submit desc
>   3. Mark for stable tree
> 
> * Changes from v1:
>   NONE
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 53 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
> 

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

Re: [PATCH v3 12/16] mtd: rawnand: qcom: fix return value for raw page read

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:40 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> 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>
> ---

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

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:41 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> The QCOM NAND controller layout is such that, the bad block byte
> offset for last codeword will come to first byte in spare area.

"is the first spare byte"?


> Currently, the raw read for last codeword is being done with
> copy_last_cw function. It does following 2 things:

"It does the following:"

> 
> 1. Read the last codeword bytes from NAND chip to NAND
>    controller internal HW buffer.
> 2. Copy all these bytes from HW buffer to actual 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.

"This patch does minor code reorganization to just retrieve these two
bytes when checking for bad blocks, allowing to remove
copy_last_cw() now useless."

> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v2:
>   1. Changed commit message and comments slightly
> 
> * 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 d693b5f..f72bc8a 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -1769,41 +1769,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)
> @@ -2118,6 +2083,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;
>  
> @@ -2128,11 +2094,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);

Are we sure there is no layout with only 1 step?

>  
>  	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]);
>  
> @@ -2141,12 +2127,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;

This is suspect as it still points to the beginning of the data buffer.
Can you please check you did not meant bbm_bytes_buf[bbpos]?

>  
>  	if (chip->options & NAND_BUSWIDTH_16)
> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
>  err:
>  	return bad;
>  }


Thanks,
Miquèl

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Miquel Raynal]

Hi Abhishek,


> @@ -2141,12 +2127,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;

BTW, as there are host->bbm_size bytes that can inform on the block
state, don't we need to check all of them?

>  
>  	if (chip->options & NAND_BUSWIDTH_16)
> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
>  err:
>  	return bad;
>  }

Thanks,
Miquèl

Re: [PATCH v3 14/16] mtd: rawnand: qcom: check for operation errors in case of raw read

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:42 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> 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>
> ---

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

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

[Masahiro Yamada]

2018-05-25 21:21 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
> Use the NAND core helper function nand_ecc_choose_conf to tune
> the ECC parameters instead of the function locally defined.
>
> CC: Masahiro Yamada <yamada.masahiro@socionext.com>

You can replace the CC with my

Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>



> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v2:
>
>   1. Changed commit message
>
> * 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..a586a1d 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_choose_conf(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
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 15/16] mtd: rawnand: qcom: helper function for raw read

[Miquel Raynal]

Hi Abhishek,

On Fri, 25 May 2018 17:51:43 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> 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 v2:
>   NONE
> 
> * 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 87f900e..34143a4 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -1588,6 +1588,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 actual 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)

Please prefix the helper with "qcom_nandc"

> +{
> +	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++) {

This comment applies for both patches 15 and 16:

I would really prefer having a qcom_nandc_read_cw_raw() that reads only
one CW. From qcom_nandc_read_page_raw() you would loop over all the CW
calling qcom_nandc_read_cw_raw() helper (it's raw reads, we don't care
about performances) and from ->read_page_raw() you would check
CW with uncorrectable errors for being blank with that helper. You
would avoid the not-so-nice logic where you read all the CW between the
first bad one and the last bad one.

Thanks,
Miquèl

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Abhishek Sahu]

On 2018-05-26 14:12, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Fri, 25 May 2018 17:51:29 +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.
>> 
>> This patch introduces nand_ecc_choose_conf 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 v2:
>> 
>>   1. Renamed function to nand_ecc_choose_conf.
>>   2. Minor code reorganization to remove warning and 2 function calls
>>      for nand_maximize_ecc.
>> 
>> * Changes from v1:
>>   NEW PATCH
>> 
>>  drivers/mtd/nand/raw/nand_base.c | 42 
>> ++++++++++++++++++++++++++++++++++++++++
>>  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
>>  include/linux/mtd/rawnand.h      |  3 +++
>>  2 files changed, 34 insertions(+)
>> 
>> diff --git a/drivers/mtd/nand/raw/nand_base.c 
>> b/drivers/mtd/nand/raw/nand_base.c
>> index 72f3a89..e52019d 100644
>> --- a/drivers/mtd/nand/raw/nand_base.c
>> +++ b/drivers/mtd/nand/raw/nand_base.c
>> @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
>>  }
>>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>> 
>> +/**
>> + * nand_ecc_choose_conf - 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 configuration 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.
>> + *
>> + * On success, the chosen ECC settings are set.
>> + */
>> +int nand_ecc_choose_conf(struct nand_chip *chip,
>> +			 const struct nand_ecc_caps *caps, int oobavail)
>> +{
>> +	if (chip->ecc.size && chip->ecc.strength)
>> +		return nand_check_ecc_caps(chip, caps, oobavail);
>> +
>> +	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
>> +	    !nand_match_ecc_req(chip, caps, oobavail))
>> +		return 0;
>> +
>> +	return nand_maximize_ecc(chip, caps, oobavail);
> 
> I personally don't mind if nand_maximize_ecc() is called twice in
> the function if it clarifies the logic. Maybe the following will be
> more clear for the user?

  Thanks Miquel.
  Both the implementations are fine.
  The above implementation (which was in Denali NAND driver) code was 
also
  clear. We can go for any of these implementation.

  Shall I update this ?

> 
> 	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;
> 
> 	return nand_maximize_ecc(chip, caps, oobavail);
> 
> Also, I'm not sure we should just error out when nand_check_ecc_caps()
> fails. What about something more robust, like:
> 

  But again, It will lead in overriding the DT ECC strength parameter.
  We started our discussion from that point. :-)

  Thanks,
  Abhishek

> 	int ret;
> 
> 	if (chip->ecc.size && chip->ecc.strength) {
> 		ret = nand_check_ecc_caps(chip, caps, oobavail);
> 		if (ret)
> 			goto maximize_ecc;
> 
> 		return 0;
> 	}
> 
> 	if (chip->ecc.options & NAND_ECC_MAXIMIZE)
> 		goto maximize_ecc;
> 
> 	ret = nand_match_ecc_req(chip, caps, oobavail);
> 	if (ret)
> 		goto maximize_ecc;
> 
> 	return 0;
> 
> maximize_ecc:
> 	return nand_maximize_ecc(chip, caps, oobavail);
> 
>> +}
>> +EXPORT_SYMBOL_GPL(nand_ecc_choose_conf);
>> +
>>  /*
>>   * 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..89889fa 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_choose_conf(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);
>> 
> 
> Thanks,
> Miquèl

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

[Abhishek Sahu]

On 2018-05-26 14:12, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Fri, 25 May 2018 17:51:31 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> If nand-ecc-strength 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 v2:
>>   NONE
>> 
>> * 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.
> 
> I think you can squash the two dt-bindings commits as they are tightly
> related to each other.
> 

  Sure Miquel.
  Earlier made one patch and then split into two.
  Will squash that and make single patch again :-)

  Thanks,
  Abhishek

>> 
>>  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 v3 05/16] mtd: rawnand: qcom: remove dt property nand-ecc-step-size

[Abhishek Sahu]

On 2018-05-26 14:12, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Fri, 25 May 2018 17:51:33 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> QCOM NAND controller supports only one step size (512) so
>> nand-ecc-step-size DT property is redundant. This property
>> can be removed and ecc step size can be assigned with 512 value.
>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v2:
>> 
>>  NEW CHANGE
>> 
>>   1. Removed the custom logic and used the helper fuction.
>>  drivers/mtd/nand/raw/qcom_nandc.c | 11 ++---------
>>  1 file changed, 2 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
>> b/drivers/mtd/nand/raw/qcom_nandc.c
>> index b554fb6..b538390 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -2325,15 +2325,8 @@ static int qcom_nand_host_setup(struct 
>> qcom_nand_host *host)
>>  	bool wide_bus;
>>  	int ecc_mode = 1;
>> 
>> -	/*
>> -	 * the controller requires each step consists of 512 bytes of data.
>> -	 * bail out if DT has populated a wrong step size.
>> -	 */
>> -	if (ecc->size != NANDC_STEP_SIZE) {
>> -		dev_err(nandc->dev, "invalid ecc size\n");
>> -		return -EINVAL;
>> -	}
>> -
>> +	/* controller only supports 512 bytes of data in each step */
> 
> "512 bytes data steps"
> 

  Thanks Miquel.
  Will update that.

  Regards,
  Abhishek

>> +	ecc->size = NANDC_STEP_SIZE;
>>  	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
>> 
>>  	if (ecc->strength >= 8) {
> 
> Once corrected:
> 
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

Re: [PATCH v3 06/16] mtd: rawnand: qcom: use the ecc strength from device parameter

[Abhishek Sahu]

On 2018-05-26 14:13, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Fri, 25 May 2018 17:51:34 +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 v2:
>>   NONE
> 
> Yes you did change things:
> 
> - s/<< 2/* 4/
> - updated the cwperpage location
> - the block handling the ecc-step-size property has been removed in a
>   previous patch
> 
> Please be careful with that, it is time consuming to review the patches
> all over again.
> 

  Sorry Miquel for that.
  I Will pay more attention to this.
  I can understand the effort require in reviewing and how this can help
  in making review quicker.

>> 
>> * Changes from v1:
>> 
>>   1. Removed the custom logic and used the helper fuction.
>> 
>>  drivers/mtd/nand/raw/qcom_nandc.c | 29 +++++++++++++++++++++--------
>>  1 file changed, 21 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
>> b/drivers/mtd/nand/raw/qcom_nandc.c
>> index b538390..7377923 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -2315,19 +2315,39 @@ 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;
>> 
>>  	/* controller only supports 512 bytes of data in each step */
>>  	ecc->size = NANDC_STEP_SIZE;
>>  	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
>> +	cwperpage = mtd->writesize / NANDC_STEP_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_choose_conf(chip, &qcom_nandc_ecc_caps,
>> +				   mtd->oobsize - cwperpage * 4);
> 
> Nitpick: could you add parenthesis around (cwperpage * 4) just for
> clarity.
> 

  Thanks Miquel.
  I will update that.

  Regards,
  Abhishek

>> +	if (ret) {
>> +		dev_err(nandc->dev, "No valid ECC settings possible\n");
>> +		return ret;
>> +	}
>> 
>>  	if (ecc->strength >= 8) {
>>  		/* 8 bit ECC defaults to BCH ECC on all platforms */
>> @@ -2396,7 +2416,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);
>> 
>> @@ -2412,12 +2431,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
> 
> Once corrected:
> 
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Thanks,
> Miquèl

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

On 2018-05-26 14:16, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Fri, 25 May 2018 17:51:41 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> The QCOM NAND controller layout is such that, the bad block byte
>> offset for last codeword will come to first byte in spare area.
> 
> "is the first spare byte"?
> 
> 
>> Currently, the raw read for last codeword is being done with
>> copy_last_cw function. It does following 2 things:
> 
> "It does the following:"
> 
>> 
>> 1. Read the last codeword bytes from NAND chip to NAND
>>    controller internal HW buffer.
>> 2. Copy all these bytes from HW buffer to actual 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.
> 
> "This patch does minor code reorganization to just retrieve these two
> bytes when checking for bad blocks, allowing to remove
> copy_last_cw() now useless."
> 

  Thanks Miquel.
  I will update all these.

>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> * Changes from v2:
>>   1. Changed commit message and comments slightly
>> 
>> * 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 d693b5f..f72bc8a 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -1769,41 +1769,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)
>> @@ -2118,6 +2083,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;
>> 
>> @@ -2128,11 +2094,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);
> 
> Are we sure there is no layout with only 1 step?

  All the layouts are such that, the BBM will come in
  first byte of spare area.

  For 4 bit ECC, the cw_size is 528 so for 2K page

   2048 - 528 * 3 = 464

  So for last CW, the 464 is BBM (i.e 2048th byte) in
  full page.

> 
>> 
>>  	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]);
>> 
>> @@ -2141,12 +2127,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;
> 
> This is suspect as it still points to the beginning of the data buffer.
> Can you please check you did not meant bbm_bytes_buf[bbpos]?
> 

  The main thing here is
  nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);

  After reading one complete CW from NAND, the data will be still
  in NAND HW buffer.

  The above register tells that we need to read data from
  bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
  and 2 byte for 16 bus width) in bbm_bytes_buf.

  So bbm_bytes_buf[0] will contain the BBM first byte.
  and bbm_bytes_buf[1] will contain the BBM second byte.

  Regards,
  Abhishek

>> 
>>  	if (chip->options & NAND_BUSWIDTH_16)
>> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
>> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
>>  err:
>>  	return bad;
>>  }
> 
> 
> Thanks,
> Miquèl

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

On 2018-05-26 14:28, Miquel Raynal wrote:
> Hi Abhishek,
> 
> 
>> @@ -2141,12 +2127,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;
> 
> BTW, as there are host->bbm_size bytes that can inform on the block
> state, don't we need to check all of them?
> 

  We are checking all of them.
  host->bbm_size will be either 1 (for NAND_BUSWIDTH_8) or
  2 (for NAND_BUSWIDTH_16).

  
https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/mtd/nand/raw/qcom_nandc.c#L2347

  Thanks,
  Abhishek

>> 
>>  	if (chip->options & NAND_BUSWIDTH_16)
>> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
>> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
>>  err:
>>  	return bad;
>>  }
> 
> Thanks,
> Miquèl

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Miquel Raynal]

Hi Abhishek,

> >>  /* 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)
> >> @@ -2118,6 +2083,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;
> >> >> @@ -2128,11 +2094,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);
> > > Are we sure there is no layout with only 1 step?  
> 
>   All the layouts are such that, the BBM will come in
>   first byte of spare area.
> 
>   For 4 bit ECC, the cw_size is 528 so for 2K page
> 
>    2048 - 528 * 3 = 464

My question was more about small page NANDs. But I suppose it works
too if ecc->steps == 1.

> 
>   So for last CW, the 464 is BBM (i.e 2048th byte) in
>   full page.
> 
> > >> >>  	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]);
> >> >> @@ -2141,12 +2127,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;
> > > This is suspect as it still points to the beginning of the data buffer.  
> > Can you please check you did not meant bbm_bytes_buf[bbpos]?
> >   
>   The main thing here is
>   nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);
> 
>   After reading one complete CW from NAND, the data will be still
>   in NAND HW buffer.
> 
>   The above register tells that we need to read data from
>   bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
>   and 2 byte for 16 bus width) in bbm_bytes_buf.

I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then
it's ok.

> 
>   So bbm_bytes_buf[0] will contain the BBM first byte.
>   and bbm_bytes_buf[1] will contain the BBM second byte.
> 
>   Regards,
>   Abhishek
> 
> >> >>  	if (chip->options & NAND_BUSWIDTH_16)  
> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);

Sorry, my mistake, I did not see the above line.

However, technically, the BBM could be located in the first, second or
last page of the block. You should check the three of them are 0xFF
before declaring the block is not bad.

The more I look at the function, the more I wonder if you actually need
it. Why does the generic nand_block_bad() implementation in the core
do not fit?

> >>  err:
> >>  	return bad;
> >>  }  
> > > > Thanks,  
> > Miquèl  

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Miquel Raynal]

Hi Abhishek,

On Mon, 28 May 2018 11:46:47 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> On 2018-05-26 14:28, Miquel Raynal wrote:
> > Hi Abhishek,  
> > > >> @@ -2141,12 +2127,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;
> > > BTW, as there are host->bbm_size bytes that can inform on the block  
> > state, don't we need to check all of them?
> >   
>   We are checking all of them.
>   host->bbm_size will be either 1 (for NAND_BUSWIDTH_8) or
>   2 (for NAND_BUSWIDTH_16).
> 
>   https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/mtd/nand/raw/qcom_nandc.c#L2347
> 
>   Thanks,
>   Abhishek
> 
> >> >>  	if (chip->options & NAND_BUSWIDTH_16)  
> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);

As told in my previous reply, I missed the above line.

However, after checking the code of the core (nand_base.c) I wonder if
it is useful to check for the second byte.

And if you look at the core's implementation you'll see that the offset
is not always 0 in the OOB but maybe 5 for small page NAND chips.

Please have a look to the generic implementation and tell me why this
is really needed?

Thanks,
Miquèl

Re: [PATCH v3 15/16] mtd: rawnand: qcom: helper function for raw read

[Abhishek Sahu]

On 2018-05-27 19:23, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Fri, 25 May 2018 17:51:43 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> 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 v2:
>>   NONE
>> 
>> * 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 87f900e..34143a4 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -1588,6 +1588,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 actual 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)
> 
> Please prefix the helper with "qcom_nandc"
> 

  Sure Miquel.
  I will update that.

>> +{
>> +	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++) {
> 
> This comment applies for both patches 15 and 16:
> 
> I would really prefer having a qcom_nandc_read_cw_raw() that reads only
> one CW. From qcom_nandc_read_page_raw() you would loop over all the CW
> calling qcom_nandc_read_cw_raw() helper (it's raw reads, we don't care
> about performances)

  Doing that way will degrade performances hugely.

  Currently once we formed the descriptor, the DMA will take care
  of complete page data transfer from NAND device to buffer and will
  generate single interrupt.

  Now it will form one CW descriptor and wait for it to be finished.
  In background, the data transfer from NAND device will be also
  split and for every CW, it will give the PAGE_READ command again,
  which is again time consuming.

  Data transfer degradation is ok but it will increase CPU time
  and number of interrupts which will impact other peripherals
  performance that time.

  Most of the NAND parts has 4K page size i.e 8 CWs.

> and from ->read_page_raw() you would check
> CW with uncorrectable errors for being blank with that helper. You
> would avoid the not-so-nice logic where you read all the CW between the
> first bad one and the last bad one.
> 

  The reading b/w first CW and last CW is only from NAND device to NAND
  HW buffers. The NAND controller has 2 HW buffers which is used to
  optimize the traffic throughput between the NAND device and
  system memory,in both directions. Each buffer is 544B in size: 512B
  for data + 32B spare bytes. Throughput optimization is achieved by
  executing internal data transfers (i.e. between NANDc buffers and
  system memory) simultaneously with NAND device operations.

  Making separate function won't help in improving performance for
  this case either since once every thing is set for reading page
  (descriptor formation, issue the PAGE_READ, Data transfer from
  Flash array to data register in NAND device), the read time from
  device to NAND HW buffer is very less. Again, we did optimization
  in which the copying from NAND HW buffer to actual buffer is being
  done only for those CW's only.

  Again, in this case CPU time will be more.

  Thanks,
  Abhishek

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

On 2018-05-28 12:33, Miquel Raynal wrote:
> Hi Abhishek,
> 
>> >>  /* 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)
>> >> @@ -2118,6 +2083,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;
>> >> >> @@ -2128,11 +2094,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);
>> > > Are we sure there is no layout with only 1 step?
>> 
>>   All the layouts are such that, the BBM will come in
>>   first byte of spare area.
>> 
>>   For 4 bit ECC, the cw_size is 528 so for 2K page
>> 
>>    2048 - 528 * 3 = 464
> 
> My question was more about small page NANDs. But I suppose it works
> too if ecc->steps == 1.
> 

  Correct Miquel.

>> 
>>   So for last CW, the 464 is BBM (i.e 2048th byte) in
>>   full page.
>> 
>> > >> >>  	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]);
>> >> >> @@ -2141,12 +2127,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;
>> > > This is suspect as it still points to the beginning of the data buffer.
>> > Can you please check you did not meant bbm_bytes_buf[bbpos]?
>> >
>>   The main thing here is
>>   nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);
>> 
>>   After reading one complete CW from NAND, the data will be still
>>   in NAND HW buffer.
>> 
>>   The above register tells that we need to read data from
>>   bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
>>   and 2 byte for 16 bus width) in bbm_bytes_buf.
> 
> I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then
> it's ok.
> 
>> 
>>   So bbm_bytes_buf[0] will contain the BBM first byte.
>>   and bbm_bytes_buf[1] will contain the BBM second byte.
>> 
>>   Regards,
>>   Abhishek
>> 
>> >> >>  	if (chip->options & NAND_BUSWIDTH_16)
>> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
>> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
> 
> Sorry, my mistake, I did not see the above line.
> 
> However, technically, the BBM could be located in the first, second or
> last page of the block. You should check the three of them are 0xFF
> before declaring the block is not bad.
> 
> The more I look at the function, the more I wonder if you actually need
> it. Why does the generic nand_block_bad() implementation in the core
> do not fit?

  The BBM bytes can be accessed in raw mode only for QCOM NAND
  Contoller. We started with following patch for initial patches

  http://patchwork.ozlabs.org/patch/508565/

  I am also not very much sure, how can we go ahead now.
  Ideally we need to use generic function only which
  requires raw_read.

  Thanks,
  Abhishek


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

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

On 2018-05-28 12:33, Miquel Raynal wrote:
> Hi Abhishek,
> 
>> >>  /* 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)
>> >> @@ -2118,6 +2083,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;
>> >> >> @@ -2128,11 +2094,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);
>> > > Are we sure there is no layout with only 1 step?
>> 
>>   All the layouts are such that, the BBM will come in
>>   first byte of spare area.
>> 
>>   For 4 bit ECC, the cw_size is 528 so for 2K page
>> 
>>    2048 - 528 * 3 = 464
> 
> My question was more about small page NANDs. But I suppose it works
> too if ecc->steps == 1.
> 

  Correct Miquel.

>> 
>>   So for last CW, the 464 is BBM (i.e 2048th byte) in
>>   full page.
>> 
>> > >> >>  	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]);
>> >> >> @@ -2141,12 +2127,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;
>> > > This is suspect as it still points to the beginning of the data buffer.
>> > Can you please check you did not meant bbm_bytes_buf[bbpos]?
>> >
>>   The main thing here is
>>   nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);
>> 
>>   After reading one complete CW from NAND, the data will be still
>>   in NAND HW buffer.
>> 
>>   The above register tells that we need to read data from
>>   bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
>>   and 2 byte for 16 bus width) in bbm_bytes_buf.
> 
> I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then
> it's ok.
> 
>> 
>>   So bbm_bytes_buf[0] will contain the BBM first byte.
>>   and bbm_bytes_buf[1] will contain the BBM second byte.
>> 
>>   Regards,
>>   Abhishek
>> 
>> >> >>  	if (chip->options & NAND_BUSWIDTH_16)
>> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
>> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
> 
> Sorry, my mistake, I did not see the above line.
> 
> However, technically, the BBM could be located in the first, second or
> last page of the block. You should check the three of them are 0xFF
> before declaring the block is not bad.
> 
> The more I look at the function, the more I wonder if you actually need
> it. Why does the generic nand_block_bad() implementation in the core
> do not fit?

  The BBM bytes can be accessed in raw mode only for QCOM NAND
  Contoller. We started with following patch for initial patches

  http://patchwork.ozlabs.org/patch/508565/

  I am also not very much sure, how can we go ahead now.
  Ideally we need to use generic function only which
  requires raw_read.

  Thanks,
  Abhishek

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Boris Brezillon]

On Sat, 26 May 2018 10:42:47 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Abhishek,
> 
> On Fri, 25 May 2018 17:51:29 +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.
> > 
> > This patch introduces nand_ecc_choose_conf 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 v2:
> > 
> >   1. Renamed function to nand_ecc_choose_conf.
> >   2. Minor code reorganization to remove warning and 2 function calls
> >      for nand_maximize_ecc.
> > 
> > * Changes from v1:
> >   NEW PATCH
> > 
> >  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
> >  include/linux/mtd/rawnand.h      |  3 +++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..e52019d 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
> >  }
> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
> >  
> > +/**
> > + * nand_ecc_choose_conf - 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 configuration 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.
> > + *
> > + * On success, the chosen ECC settings are set.
> > + */
> > +int nand_ecc_choose_conf(struct nand_chip *chip,
> > +			 const struct nand_ecc_caps *caps, int oobavail)
> > +{
> > +	if (chip->ecc.size && chip->ecc.strength)
> > +		return nand_check_ecc_caps(chip, caps, oobavail);
> > +
> > +	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
> > +	    !nand_match_ecc_req(chip, caps, oobavail))
> > +		return 0;
> > +
> > +	return nand_maximize_ecc(chip, caps, oobavail);  
> 
> I personally don't mind if nand_maximize_ecc() is called twice in
> the function if it clarifies the logic. Maybe the following will be
> more clear for the user?
> 
> 	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;
> 
> 	return nand_maximize_ecc(chip, caps, oobavail);

I personally don't mind, and it seems Masahiro wanted to keep the logic
he had used in the denali driver.

> 
> Also, I'm not sure we should just error out when nand_check_ecc_caps()
> fails. What about something more robust, like:
> 
> 	int ret;
> 
> 	if (chip->ecc.size && chip->ecc.strength) {
> 		ret = nand_check_ecc_caps(chip, caps, oobavail);
> 		if (ret)
> 			goto maximize_ecc;

Nope. When someone asked for a specific ECC config by passing the
nand-ecc-xxx props we should apply it or return an erro if it's not
supported. People passing those props should now what the ECC engine
supports and pick one valid values.

> 
> 		return 0;
> 	}
> 
> 	if (chip->ecc.options & NAND_ECC_MAXIMIZE)
> 		goto maximize_ecc;
> 
> 	ret = nand_match_ecc_req(chip, caps, oobavail);
> 	if (ret)
> 		goto maximize_ecc;
> 
> 	return 0;
> 
> maximize_ecc:
> 	return nand_maximize_ecc(chip, caps, oobavail);
> 

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Masahiro Yamada]

Hi.

2018-05-30 4:30 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Sat, 26 May 2018 10:42:47 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>> Hi Abhishek,
>>
>> On Fri, 25 May 2018 17:51:29 +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.
>> >
>> > This patch introduces nand_ecc_choose_conf 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 v2:
>> >
>> >   1. Renamed function to nand_ecc_choose_conf.
>> >   2. Minor code reorganization to remove warning and 2 function calls
>> >      for nand_maximize_ecc.
>> >
>> > * Changes from v1:
>> >   NEW PATCH
>> >
>> >  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
>> >  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
>> >  include/linux/mtd/rawnand.h      |  3 +++
>> >  2 files changed, 34 insertions(+)
>> >
>> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
>> > index 72f3a89..e52019d 100644
>> > --- a/drivers/mtd/nand/raw/nand_base.c
>> > +++ b/drivers/mtd/nand/raw/nand_base.c
>> > @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
>> >  }
>> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>> >
>> > +/**
>> > + * nand_ecc_choose_conf - 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 configuration 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.
>> > + *
>> > + * On success, the chosen ECC settings are set.
>> > + */
>> > +int nand_ecc_choose_conf(struct nand_chip *chip,
>> > +                    const struct nand_ecc_caps *caps, int oobavail)
>> > +{
>> > +   if (chip->ecc.size && chip->ecc.strength)
>> > +           return nand_check_ecc_caps(chip, caps, oobavail);
>> > +
>> > +   if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
>> > +       !nand_match_ecc_req(chip, caps, oobavail))
>> > +           return 0;
>> > +
>> > +   return nand_maximize_ecc(chip, caps, oobavail);
>>
>> I personally don't mind if nand_maximize_ecc() is called twice in
>> the function if it clarifies the logic. Maybe the following will be
>> more clear for the user?
>>
>>       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;
>>
>>       return nand_maximize_ecc(chip, caps, oobavail);
>
> I personally don't mind, and it seems Masahiro wanted to keep the logic
> he had used in the denali driver.
>
>>
>> Also, I'm not sure we should just error out when nand_check_ecc_caps()
>> fails. What about something more robust, like:
>>
>>       int ret;
>>
>>       if (chip->ecc.size && chip->ecc.strength) {
>>               ret = nand_check_ecc_caps(chip, caps, oobavail);
>>               if (ret)
>>                       goto maximize_ecc;
>
> Nope. When someone asked for a specific ECC config by passing the
> nand-ecc-xxx props we should apply it or return an erro if it's not
> supported. People passing those props should now what the ECC engine
> supports and pick one valid values.
>
>>
>>               return 0;
>>       }
>>
>>       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>>               goto maximize_ecc;
>>
>>       ret = nand_match_ecc_req(chip, caps, oobavail);
>>       if (ret)
>>               goto maximize_ecc;
>>
>>       return 0;
>>
>> maximize_ecc:
>>       return nand_maximize_ecc(chip, caps, oobavail);
>>
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/






This version looks good to me.

If you want to check the error code more precisely,
how about something like follows?



int nand_ecc_choose_conf(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)) {
               ret = nand_match_ecc_req(chip, caps, oobavail);
               if (ret != -ENOTSUPP)
                       return ret;
       }

       return nand_maximize_ecc(chip, caps, oobavail);
}


Only the difference is the case
where nand_match_ecc_req() returns a different error code
than ENOTSUPP.
(Currently, this happens only when insane 'oobavail' is passed.)


ENOTSUPP means 'required ECC setting is not supported'.
Other error code is more significant, so it is not a good reason
to fall back to miximization, IMHO.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Abhishek Sahu]

On 2018-05-30 05:58, Masahiro Yamada wrote:
> Hi.
> 
> 2018-05-30 4:30 GMT+09:00 Boris Brezillon 
> <boris.brezillon@bootlin.com>:
>> On Sat, 26 May 2018 10:42:47 +0200
>> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>> 
>>> Hi Abhishek,
>>> 
>>> On Fri, 25 May 2018 17:51:29 +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.
>>> >
>>> > This patch introduces nand_ecc_choose_conf 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 v2:
>>> >
>>> >   1. Renamed function to nand_ecc_choose_conf.
>>> >   2. Minor code reorganization to remove warning and 2 function calls
>>> >      for nand_maximize_ecc.
>>> >
>>> > * Changes from v1:
>>> >   NEW PATCH
>>> >
>>> >  drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++
>>> >  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
>>> >  include/linux/mtd/rawnand.h      |  3 +++
>>> >  2 files changed, 34 insertions(+)
>>> >
>>> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
>>> > index 72f3a89..e52019d 100644
>>> > --- a/drivers/mtd/nand/raw/nand_base.c
>>> > +++ b/drivers/mtd/nand/raw/nand_base.c
>>> > @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
>>> >  }
>>> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>>> >
>>> > +/**
>>> > + * nand_ecc_choose_conf - 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 configuration 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.
>>> > + *
>>> > + * On success, the chosen ECC settings are set.
>>> > + */
>>> > +int nand_ecc_choose_conf(struct nand_chip *chip,
>>> > +                    const struct nand_ecc_caps *caps, int oobavail)
>>> > +{
>>> > +   if (chip->ecc.size && chip->ecc.strength)
>>> > +           return nand_check_ecc_caps(chip, caps, oobavail);
>>> > +
>>> > +   if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
>>> > +       !nand_match_ecc_req(chip, caps, oobavail))
>>> > +           return 0;
>>> > +
>>> > +   return nand_maximize_ecc(chip, caps, oobavail);
>>> 
>>> I personally don't mind if nand_maximize_ecc() is called twice in
>>> the function if it clarifies the logic. Maybe the following will be
>>> more clear for the user?
>>> 
>>>       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;
>>> 
>>>       return nand_maximize_ecc(chip, caps, oobavail);
>> 
>> I personally don't mind, and it seems Masahiro wanted to keep the 
>> logic
>> he had used in the denali driver.
>> 
>>> 
>>> Also, I'm not sure we should just error out when 
>>> nand_check_ecc_caps()
>>> fails. What about something more robust, like:
>>> 
>>>       int ret;
>>> 
>>>       if (chip->ecc.size && chip->ecc.strength) {
>>>               ret = nand_check_ecc_caps(chip, caps, oobavail);
>>>               if (ret)
>>>                       goto maximize_ecc;
>> 
>> Nope. When someone asked for a specific ECC config by passing the
>> nand-ecc-xxx props we should apply it or return an erro if it's not
>> supported. People passing those props should now what the ECC engine
>> supports and pick one valid values.
>> 
>>> 
>>>               return 0;
>>>       }
>>> 
>>>       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>>>               goto maximize_ecc;
>>> 
>>>       ret = nand_match_ecc_req(chip, caps, oobavail);
>>>       if (ret)
>>>               goto maximize_ecc;
>>> 
>>>       return 0;
>>> 
>>> maximize_ecc:
>>>       return nand_maximize_ecc(chip, caps, oobavail);
>>> 
>> 
>> 
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 
> 
> 
> 
> 
> This version looks good to me.
> 
> If you want to check the error code more precisely,
> how about something like follows?
> 
> 
> 
> int nand_ecc_choose_conf(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)) {
>                ret = nand_match_ecc_req(chip, caps, oobavail);
>                if (ret != -ENOTSUPP)
>                        return ret;
>        }
> 
>        return nand_maximize_ecc(chip, caps, oobavail);
> }
> 
> 
> Only the difference is the case
> where nand_match_ecc_req() returns a different error code
> than ENOTSUPP.
> (Currently, this happens only when insane 'oobavail' is passed.)
> 

  We can do that but to me, it will make the helper function
  more complicated. Currently, nand_match_ecc_req is returning
  other than ENOTSUPP 'oobavail < 0' is passed.
  and again in nand_maximize_ecc, we will check for validity
  of oobavail so nothing wrong will happen in calling
  nand_maximize_ecc.

  Anyway we put this under WARN_ON condition

         if (WARN_ON(oobavail < 0))
                 return -EINVAL;

  so if this is being triggered, then it should be mostly
  programming error.

  Thanks,
  Abhishek

> 
> ENOTSUPP means 'required ECC setting is not supported'.
> Other error code is more significant, so it is not a good reason
> to fall back to miximization, IMHO.

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Masahiro Yamada]

2018-05-30 15:21 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
> On 2018-05-30 05:58, Masahiro Yamada wrote:
>>
>> Hi.
>>
>> 2018-05-30 4:30 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
>>>
>>> On Sat, 26 May 2018 10:42:47 +0200
>>> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>>
>>>> Hi Abhishek,
>>>>
>>>> On Fri, 25 May 2018 17:51:29 +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.
>>>> >
>>>> > This patch introduces nand_ecc_choose_conf 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 v2:
>>>> >
>>>> >   1. Renamed function to nand_ecc_choose_conf.
>>>> >   2. Minor code reorganization to remove warning and 2 function calls
>>>> >      for nand_maximize_ecc.
>>>> >
>>>> > * Changes from v1:
>>>> >   NEW PATCH
>>>> >
>>>> >  drivers/mtd/nand/raw/nand_base.c | 42
>>>> > ++++++++++++++++++++++++++++++++++++++++
>>>> >  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
>>>> >  include/linux/mtd/rawnand.h      |  3 +++
>>>> >  2 files changed, 34 insertions(+)
>>>> >
>>>> > diff --git a/drivers/mtd/nand/raw/nand_base.c
>>>> > b/drivers/mtd/nand/raw/nand_base.c
>>>> > index 72f3a89..e52019d 100644
>>>> > --- a/drivers/mtd/nand/raw/nand_base.c
>>>> > +++ b/drivers/mtd/nand/raw/nand_base.c
>>>> > @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
>>>> >  }
>>>> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>>>> >
>>>> > +/**
>>>> > + * nand_ecc_choose_conf - 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 configuration 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.
>>>> > + *
>>>> > + * On success, the chosen ECC settings are set.
>>>> > + */
>>>> > +int nand_ecc_choose_conf(struct nand_chip *chip,
>>>> > +                    const struct nand_ecc_caps *caps, int oobavail)
>>>> > +{
>>>> > +   if (chip->ecc.size && chip->ecc.strength)
>>>> > +           return nand_check_ecc_caps(chip, caps, oobavail);
>>>> > +
>>>> > +   if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
>>>> > +       !nand_match_ecc_req(chip, caps, oobavail))
>>>> > +           return 0;
>>>> > +
>>>> > +   return nand_maximize_ecc(chip, caps, oobavail);
>>>>
>>>> I personally don't mind if nand_maximize_ecc() is called twice in
>>>> the function if it clarifies the logic. Maybe the following will be
>>>> more clear for the user?
>>>>
>>>>       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;
>>>>
>>>>       return nand_maximize_ecc(chip, caps, oobavail);
>>>
>>>
>>> I personally don't mind, and it seems Masahiro wanted to keep the logic
>>> he had used in the denali driver.
>>>
>>>>
>>>> Also, I'm not sure we should just error out when nand_check_ecc_caps()
>>>> fails. What about something more robust, like:
>>>>
>>>>       int ret;
>>>>
>>>>       if (chip->ecc.size && chip->ecc.strength) {
>>>>               ret = nand_check_ecc_caps(chip, caps, oobavail);
>>>>               if (ret)
>>>>                       goto maximize_ecc;
>>>
>>>
>>> Nope. When someone asked for a specific ECC config by passing the
>>> nand-ecc-xxx props we should apply it or return an erro if it's not
>>> supported. People passing those props should now what the ECC engine
>>> supports and pick one valid values.
>>>
>>>>
>>>>               return 0;
>>>>       }
>>>>
>>>>       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>>>>               goto maximize_ecc;
>>>>
>>>>       ret = nand_match_ecc_req(chip, caps, oobavail);
>>>>       if (ret)
>>>>               goto maximize_ecc;
>>>>
>>>>       return 0;
>>>>
>>>> maximize_ecc:
>>>>       return nand_maximize_ecc(chip, caps, oobavail);
>>>>
>>>
>>>
>>> ______________________________________________________
>>> Linux MTD discussion mailing list
>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>>
>>
>>
>>
>>
>>
>> This version looks good to me.
>>
>> If you want to check the error code more precisely,
>> how about something like follows?
>>
>>
>>
>> int nand_ecc_choose_conf(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)) {
>>                ret = nand_match_ecc_req(chip, caps, oobavail);
>>                if (ret != -ENOTSUPP)
>>                        return ret;
>>        }
>>
>>        return nand_maximize_ecc(chip, caps, oobavail);
>> }
>>
>>
>> Only the difference is the case
>> where nand_match_ecc_req() returns a different error code
>> than ENOTSUPP.
>> (Currently, this happens only when insane 'oobavail' is passed.)
>>
>
>  We can do that but to me, it will make the helper function
>  more complicated. Currently, nand_match_ecc_req is returning
>  other than ENOTSUPP 'oobavail < 0' is passed.
>  and again in nand_maximize_ecc, we will check for validity
>  of oobavail so nothing wrong will happen in calling
>  nand_maximize_ecc.


Right.  When I added those three helpers,
I supposed they were independent APIs.
That is why I added the 'oobavail < 0' sanity check
in each of the three.


If you make them internal sub-helpers
(i.e. add 'static' instead of EXPORT_SYMBOL_GPL),
you can check 'oobavail < 0'
only in nand_ecc_choose_conf().





>  Anyway we put this under WARN_ON condition
>
>         if (WARN_ON(oobavail < 0))
>                 return -EINVAL;
>
>  so if this is being triggered, then it should be mostly
>  programming error.


Right.  Moreover,

WARN_ON(oobavail < 0 || oobavail > mtd->oobsize)



This is programming error, that is why WARN_ON() is used to
make the log noisy.


>  Thanks,
>  Abhishek
>
>>
>> ENOTSUPP means 'required ECC setting is not supported'.
>> Other error code is more significant, so it is not a good reason
>> to fall back to miximization, IMHO.
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Abhishek Sahu]

On 2018-05-30 13:08, Masahiro Yamada wrote:
> 2018-05-30 15:21 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
>> On 2018-05-30 05:58, Masahiro Yamada wrote:
>>> 
>>> Hi.
>>> 
>>> 2018-05-30 4:30 GMT+09:00 Boris Brezillon 
>>> <boris.brezillon@bootlin.com>:
>>>> 
>>>> On Sat, 26 May 2018 10:42:47 +0200
>>>> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>>> 
>>>>> Hi Abhishek,
>>>>> 
>>>>> On Fri, 25 May 2018 17:51:29 +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.
>>>>> >
>>>>> > This patch introduces nand_ecc_choose_conf 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 v2:
>>>>> >
>>>>> >   1. Renamed function to nand_ecc_choose_conf.
>>>>> >   2. Minor code reorganization to remove warning and 2 function calls
>>>>> >      for nand_maximize_ecc.
>>>>> >
>>>>> > * Changes from v1:
>>>>> >   NEW PATCH
>>>>> >
>>>>> >  drivers/mtd/nand/raw/nand_base.c | 42
>>>>> > ++++++++++++++++++++++++++++++++++++++++
>>>>> >  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
>>>>> >  include/linux/mtd/rawnand.h      |  3 +++
>>>>> >  2 files changed, 34 insertions(+)
>>>>> >
>>>>> > diff --git a/drivers/mtd/nand/raw/nand_base.c
>>>>> > b/drivers/mtd/nand/raw/nand_base.c
>>>>> > index 72f3a89..e52019d 100644
>>>>> > --- a/drivers/mtd/nand/raw/nand_base.c
>>>>> > +++ b/drivers/mtd/nand/raw/nand_base.c
>>>>> > @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
>>>>> >  }
>>>>> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>>>>> >
>>>>> > +/**
>>>>> > + * nand_ecc_choose_conf - 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 configuration 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.
>>>>> > + *
>>>>> > + * On success, the chosen ECC settings are set.
>>>>> > + */
>>>>> > +int nand_ecc_choose_conf(struct nand_chip *chip,
>>>>> > +                    const struct nand_ecc_caps *caps, int oobavail)
>>>>> > +{
>>>>> > +   if (chip->ecc.size && chip->ecc.strength)
>>>>> > +           return nand_check_ecc_caps(chip, caps, oobavail);
>>>>> > +
>>>>> > +   if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
>>>>> > +       !nand_match_ecc_req(chip, caps, oobavail))
>>>>> > +           return 0;
>>>>> > +
>>>>> > +   return nand_maximize_ecc(chip, caps, oobavail);
>>>>> 
>>>>> I personally don't mind if nand_maximize_ecc() is called twice in
>>>>> the function if it clarifies the logic. Maybe the following will be
>>>>> more clear for the user?
>>>>> 
>>>>>       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;
>>>>> 
>>>>>       return nand_maximize_ecc(chip, caps, oobavail);
>>>> 
>>>> 
>>>> I personally don't mind, and it seems Masahiro wanted to keep the 
>>>> logic
>>>> he had used in the denali driver.
>>>> 
>>>>> 
>>>>> Also, I'm not sure we should just error out when 
>>>>> nand_check_ecc_caps()
>>>>> fails. What about something more robust, like:
>>>>> 
>>>>>       int ret;
>>>>> 
>>>>>       if (chip->ecc.size && chip->ecc.strength) {
>>>>>               ret = nand_check_ecc_caps(chip, caps, oobavail);
>>>>>               if (ret)
>>>>>                       goto maximize_ecc;
>>>> 
>>>> 
>>>> Nope. When someone asked for a specific ECC config by passing the
>>>> nand-ecc-xxx props we should apply it or return an erro if it's not
>>>> supported. People passing those props should now what the ECC engine
>>>> supports and pick one valid values.
>>>> 
>>>>> 
>>>>>               return 0;
>>>>>       }
>>>>> 
>>>>>       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>>>>>               goto maximize_ecc;
>>>>> 
>>>>>       ret = nand_match_ecc_req(chip, caps, oobavail);
>>>>>       if (ret)
>>>>>               goto maximize_ecc;
>>>>> 
>>>>>       return 0;
>>>>> 
>>>>> maximize_ecc:
>>>>>       return nand_maximize_ecc(chip, caps, oobavail);
>>>>> 
>>>> 
>>>> 
>>>> ______________________________________________________
>>>> Linux MTD discussion mailing list
>>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> This version looks good to me.
>>> 
>>> If you want to check the error code more precisely,
>>> how about something like follows?
>>> 
>>> 
>>> 
>>> int nand_ecc_choose_conf(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)) {
>>>                ret = nand_match_ecc_req(chip, caps, oobavail);
>>>                if (ret != -ENOTSUPP)
>>>                        return ret;
>>>        }
>>> 
>>>        return nand_maximize_ecc(chip, caps, oobavail);
>>> }
>>> 
>>> 
>>> Only the difference is the case
>>> where nand_match_ecc_req() returns a different error code
>>> than ENOTSUPP.
>>> (Currently, this happens only when insane 'oobavail' is passed.)
>>> 
>> 
>>  We can do that but to me, it will make the helper function
>>  more complicated. Currently, nand_match_ecc_req is returning
>>  other than ENOTSUPP 'oobavail < 0' is passed.
>>  and again in nand_maximize_ecc, we will check for validity
>>  of oobavail so nothing wrong will happen in calling
>>  nand_maximize_ecc.
> 
> 
> Right.  When I added those three helpers,
> I supposed they were independent APIs.
> That is why I added the 'oobavail < 0' sanity check
> in each of the three.
> 
> 
> If you make them internal sub-helpers
> (i.e. add 'static' instead of EXPORT_SYMBOL_GPL),
> you can check 'oobavail < 0'
> only in nand_ecc_choose_conf().
> 
> 

  I am not sure regarding making them static.

  Currently, Denali NAND driver is only using these functions.

  And Now, this nand_ecc_choose_conf will be help
  in all the cases.

  For nand_check_ecc_caps: call nand_ecc_choose_conf with
                           chip->ecc.size && chip->ecc.strength

  For nand_maximize_ecc: call nand_ecc_choose_conf with
                         NAND_ECC_MAXIMIZE

  So making them static also seems ok which will be
  easy to maintain in future.

  Thanks,
  Abhishek

> 
> 
> 
>>  Anyway we put this under WARN_ON condition
>> 
>>         if (WARN_ON(oobavail < 0))
>>                 return -EINVAL;
>> 
>>  so if this is being triggered, then it should be mostly
>>  programming error.
> 
> 
> Right.  Moreover,
> 
> WARN_ON(oobavail < 0 || oobavail > mtd->oobsize)
> 
> 
> 
> This is programming error, that is why WARN_ON() is used to
> make the log noisy.
> 
> 
>>  Thanks,
>>  Abhishek
>> 
>>> 
>>> ENOTSUPP means 'required ECC setting is not supported'.
>>> Other error code is more significant, so it is not a good reason
>>> to fall back to miximization, IMHO.
>> 
>> 
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Miquel Raynal]

Hi Abhishek,

On Mon, 28 May 2018 11:16:29 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> On 2018-05-26 14:12, Miquel Raynal wrote:
> > Hi Abhishek,  
> > > On Fri, 25 May 2018 17:51:29 +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.  
> >> >> This patch introduces nand_ecc_choose_conf 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 v2:  
> >> >>   1. Renamed function to nand_ecc_choose_conf.  
> >>   2. Minor code reorganization to remove warning and 2 function calls
> >>      for nand_maximize_ecc.  
> >> >> * Changes from v1:  
> >>   NEW PATCH  
> >> >>  drivers/mtd/nand/raw/nand_base.c | 42 >> ++++++++++++++++++++++++++++++++++++++++  
> >>  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
> >>  include/linux/mtd/rawnand.h      |  3 +++
> >>  2 files changed, 34 insertions(+)  
> >> >> diff --git a/drivers/mtd/nand/raw/nand_base.c >> b/drivers/mtd/nand/raw/nand_base.c  
> >> index 72f3a89..e52019d 100644
> >> --- a/drivers/mtd/nand/raw/nand_base.c
> >> +++ b/drivers/mtd/nand/raw/nand_base.c
> >> @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
> >>  }
> >>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);  
> >> >> +/**  
> >> + * nand_ecc_choose_conf - 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 configuration 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.
> >> + *
> >> + * On success, the chosen ECC settings are set.
> >> + */
> >> +int nand_ecc_choose_conf(struct nand_chip *chip,
> >> +			 const struct nand_ecc_caps *caps, int oobavail)
> >> +{
> >> +	if (chip->ecc.size && chip->ecc.strength)
> >> +		return nand_check_ecc_caps(chip, caps, oobavail);
> >> +
> >> +	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
> >> +	    !nand_match_ecc_req(chip, caps, oobavail))
> >> +		return 0;
> >> +
> >> +	return nand_maximize_ecc(chip, caps, oobavail);
> > > I personally don't mind if nand_maximize_ecc() is called twice in  
> > the function if it clarifies the logic. Maybe the following will be
> > more clear for the user?  
> 
>   Thanks Miquel.
>   Both the implementations are fine.
>   The above implementation (which was in Denali NAND driver) code was also
>   clear. We can go for any of these implementation.
> 
>   Shall I update this ?

Yes, please :)

> 
> > > 	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;  
> > > 	return nand_maximize_ecc(chip, caps, oobavail);
> > > Also, I'm not sure we should just error out when nand_check_ecc_caps()  
> > fails. What about something more robust, like:
> >   
>   But again, It will lead in overriding the DT ECC strength parameter.
>   We started our discussion from that point. :-)

As Boris said, let's error out instead of overriding the DT ECC
parameters.


Thanks,
Miquèl

Re: [PATCH v3 15/16] mtd: rawnand: qcom: helper function for raw read

[Miquel Raynal]

Hi Abhishek,

On Mon, 28 May 2018 13:04:45 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> On 2018-05-27 19:23, Miquel Raynal wrote:
> > Hi Abhishek,  
> > > On Fri, 25 May 2018 17:51:43 +0530, Abhishek Sahu  
> > <absahu@codeaurora.org> wrote:  
> > >> 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 v2:
> >>   NONE  
> >> >> * 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 87f900e..34143a4 100644
> >> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> >> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> >> @@ -1588,6 +1588,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 actual 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)
> > > Please prefix the helper with "qcom_nandc"  
> >   
>   Sure Miquel.
>   I will update that.
> 
> >> +{
> >> +	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++) {
> > > This comment applies for both patches 15 and 16:
> > > I would really prefer having a qcom_nandc_read_cw_raw() that reads only  
> > one CW. From qcom_nandc_read_page_raw() you would loop over all the CW
> > calling qcom_nandc_read_cw_raw() helper (it's raw reads, we don't care
> > about performances)  
> 
>   Doing that way will degrade performances hugely.
> 
>   Currently once we formed the descriptor, the DMA will take care
>   of complete page data transfer from NAND device to buffer and will
>   generate single interrupt.
> 
>   Now it will form one CW descriptor and wait for it to be finished.
>   In background, the data transfer from NAND device will be also
>   split and for every CW, it will give the PAGE_READ command again,
>   which is again time consuming.
> 
>   Data transfer degradation is ok but it will increase CPU time
>   and number of interrupts which will impact other peripherals
>   performance that time.
> 
>   Most of the NAND parts has 4K page size i.e 8 CWs.
> 
> > and from ->read_page_raw() you would check
> > CW with uncorrectable errors for being blank with that helper. You
> > would avoid the not-so-nice logic where you read all the CW between the
> > first bad one and the last bad one.
> >   
>   The reading b/w first CW and last CW is only from NAND device to NAND
>   HW buffers. The NAND controller has 2 HW buffers which is used to
>   optimize the traffic throughput between the NAND device and
>   system memory,in both directions. Each buffer is 544B in size: 512B
>   for data + 32B spare bytes. Throughput optimization is achieved by
>   executing internal data transfers (i.e. between NANDc buffers and
>   system memory) simultaneously with NAND device operations.
> 
>   Making separate function won't help in improving performance for
>   this case either since once every thing is set for reading page
>   (descriptor formation, issue the PAGE_READ, Data transfer from
>   Flash array to data register in NAND device), the read time from
>   device to NAND HW buffer is very less. Again, we did optimization
>   in which the copying from NAND HW buffer to actual buffer is being
>   done only for those CW's only.
> 
>   Again, in this case CPU time will be more.
> 


I understand the point and thanks for detailing it. But raw access
happen either during debug (we don't care about CPU time) or when there
is an uncorrectable error, which is very unlikely to happen very often
when using eg. UBI/UBIFS. So I'm still convinced it is better to have a
_simple_ and straightforward code for this path than something way
harder to understand and much faster.

You can add a comment to explain what would be the fastest way and
why though.


Thanks,
Miquèl

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Miquel Raynal]

Hi Abhishek,

On Mon, 28 May 2018 15:40:52 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> On 2018-05-28 12:33, Miquel Raynal wrote:
> > Hi Abhishek,  
> > >> >>  /* 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)
> >> >> @@ -2118,6 +2083,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;
> >> >> >> @@ -2128,11 +2094,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);
> >> > > Are we sure there is no layout with only 1 step?
> >> >>   All the layouts are such that, the BBM will come in  
> >>   first byte of spare area.  
> >> >>   For 4 bit ECC, the cw_size is 528 so for 2K page
> >> >>    2048 - 528 * 3 = 464  
> > > My question was more about small page NANDs. But I suppose it works  
> > too if ecc->steps == 1.
> >   
>   Correct Miquel.
> 
> >> >>   So for last CW, the 464 is BBM (i.e 2048th byte) in  
> >>   full page.  
> >> >> > >> >>  	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]);
> >> >> >> @@ -2141,12 +2127,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;
> >> > > This is suspect as it still points to the beginning of the data buffer.  
> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]?
> >> >  
> >>   The main thing here is
> >>   nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);  
> >> >>   After reading one complete CW from NAND, the data will be still  
> >>   in NAND HW buffer.  
> >> >>   The above register tells that we need to read data from  
> >>   bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
> >>   and 2 byte for 16 bus width) in bbm_bytes_buf.
> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then  
> > it's ok.  
> > >> >>   So bbm_bytes_buf[0] will contain the BBM first byte.  
> >>   and bbm_bytes_buf[1] will contain the BBM second byte.  
> >> >>   Regards,  
> >>   Abhishek  
> >> >> >> >>  	if (chip->options & NAND_BUSWIDTH_16)  
> >> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
> >> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);  
> > > Sorry, my mistake, I did not see the above line.
> > > However, technically, the BBM could be located in the first, second or  
> > last page of the block. You should check the three of them are 0xFF
> > before declaring the block is not bad.  
> > > The more I look at the function, the more I wonder if you actually need  
> > it. Why does the generic nand_block_bad() implementation in the core
> > do not fit?  
> 
>   The BBM bytes can be accessed in raw mode only for QCOM NAND
>   Contoller. We started with following patch for initial patches
> 
>   http://patchwork.ozlabs.org/patch/508565/
> 
>   I am also not very much sure, how can we go ahead now.
>   Ideally we need to use generic function only which
>   requires raw_read.
> 

I see, thanks for pointing this thread.

Well for now then let's keep our driver-specific implementation.

I will just ask you to do a consistent check as requested above (you
can copy code from the core) and add a comment above this function
explaining why it is needed (what you just told me).

Thanks,
Miquèl

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Abhishek Sahu]

On 2018-06-07 18:07, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Mon, 28 May 2018 11:16:29 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> On 2018-05-26 14:12, Miquel Raynal wrote:
>> > Hi Abhishek,
>> > > On Fri, 25 May 2018 17:51:29 +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.
>> >> >> This patch introduces nand_ecc_choose_conf 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 v2:
>> >> >>   1. Renamed function to nand_ecc_choose_conf.
>> >>   2. Minor code reorganization to remove warning and 2 function calls
>> >>      for nand_maximize_ecc.
>> >> >> * Changes from v1:
>> >>   NEW PATCH
>> >> >>  drivers/mtd/nand/raw/nand_base.c | 42 >> ++++++++++++++++++++++++++++++++++++++++
>> >>  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
>> >>  include/linux/mtd/rawnand.h      |  3 +++
>> >>  2 files changed, 34 insertions(+)
>> >> >> diff --git a/drivers/mtd/nand/raw/nand_base.c >> b/drivers/mtd/nand/raw/nand_base.c
>> >> index 72f3a89..e52019d 100644
>> >> --- a/drivers/mtd/nand/raw/nand_base.c
>> >> +++ b/drivers/mtd/nand/raw/nand_base.c
>> >> @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>> >> >> +/**
>> >> + * nand_ecc_choose_conf - 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 configuration 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.
>> >> + *
>> >> + * On success, the chosen ECC settings are set.
>> >> + */
>> >> +int nand_ecc_choose_conf(struct nand_chip *chip,
>> >> +			 const struct nand_ecc_caps *caps, int oobavail)
>> >> +{
>> >> +	if (chip->ecc.size && chip->ecc.strength)
>> >> +		return nand_check_ecc_caps(chip, caps, oobavail);
>> >> +
>> >> +	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
>> >> +	    !nand_match_ecc_req(chip, caps, oobavail))
>> >> +		return 0;
>> >> +
>> >> +	return nand_maximize_ecc(chip, caps, oobavail);
>> > > I personally don't mind if nand_maximize_ecc() is called twice in
>> > the function if it clarifies the logic. Maybe the following will be
>> > more clear for the user?
>> 
>>   Thanks Miquel.
>>   Both the implementations are fine.
>>   The above implementation (which was in Denali NAND driver) code was 
>> also
>>   clear. We can go for any of these implementation.
>> 
>>   Shall I update this ?
> 
> Yes, please :)
> 

  Thanks Miquel for confirming.
  I will update accordingly.

  Also, one more question.

  Shall I make other functions (nand_check_ecc_caps, nand_maximize_ecc
  and nand_match_ecc_req) static.  Since currently,
  Denali NAND driver was only using these functions.

  And Now, this nand_ecc_choose_conf will be help
  in all the cases.

  For nand_check_ecc_caps: call nand_ecc_choose_conf with
                            chip->ecc.size && chip->ecc.strength

  For nand_maximize_ecc: call nand_ecc_choose_conf with
                         NAND_ECC_MAXIMIZE

  For other cases, nand_match_ecc_req will be called.

  So we will have one external function which will be
  easy to maintain in future.

  Thanks,
  Abhishek

>> 
>> > > 	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;
>> > > 	return nand_maximize_ecc(chip, caps, oobavail);
>> > > Also, I'm not sure we should just error out when nand_check_ecc_caps()
>> > fails. What about something more robust, like:
>> >
>>   But again, It will lead in overriding the DT ECC strength parameter.
>>   We started our discussion from that point. :-)
> 
> As Boris said, let's error out instead of overriding the DT ECC
> parameters.
> 
> 
> Thanks,
> Miquèl

Re: [PATCH v3 15/16] mtd: rawnand: qcom: helper function for raw read

[Abhishek Sahu]

On 2018-06-07 18:13, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Mon, 28 May 2018 13:04:45 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> On 2018-05-27 19:23, Miquel Raynal wrote:
>> > Hi Abhishek,
>> > > On Fri, 25 May 2018 17:51:43 +0530, Abhishek Sahu
>> > <absahu@codeaurora.org> wrote:
>> > >> 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>
>> >> ---

  <snip>

>> >> +	for (i = start_step; i < last_step; i++) {
>> > > This comment applies for both patches 15 and 16:
>> > > I would really prefer having a qcom_nandc_read_cw_raw() that reads only
>> > one CW. From qcom_nandc_read_page_raw() you would loop over all the CW
>> > calling qcom_nandc_read_cw_raw() helper (it's raw reads, we don't care
>> > about performances)
>> 
>>   Doing that way will degrade performances hugely.
>> 
>>   Currently once we formed the descriptor, the DMA will take care
>>   of complete page data transfer from NAND device to buffer and will
>>   generate single interrupt.
>> 
>>   Now it will form one CW descriptor and wait for it to be finished.
>>   In background, the data transfer from NAND device will be also
>>   split and for every CW, it will give the PAGE_READ command again,
>>   which is again time consuming.
>> 
>>   Data transfer degradation is ok but it will increase CPU time
>>   and number of interrupts which will impact other peripherals
>>   performance that time.
>> 
>>   Most of the NAND parts has 4K page size i.e 8 CWs.
>> 
>> > and from ->read_page_raw() you would check
>> > CW with uncorrectable errors for being blank with that helper. You
>> > would avoid the not-so-nice logic where you read all the CW between the
>> > first bad one and the last bad one.
>> >
>>   The reading b/w first CW and last CW is only from NAND device to 
>> NAND
>>   HW buffers. The NAND controller has 2 HW buffers which is used to
>>   optimize the traffic throughput between the NAND device and
>>   system memory,in both directions. Each buffer is 544B in size: 512B
>>   for data + 32B spare bytes. Throughput optimization is achieved by
>>   executing internal data transfers (i.e. between NANDc buffers and
>>   system memory) simultaneously with NAND device operations.
>> 
>>   Making separate function won't help in improving performance for
>>   this case either since once every thing is set for reading page
>>   (descriptor formation, issue the PAGE_READ, Data transfer from
>>   Flash array to data register in NAND device), the read time from
>>   device to NAND HW buffer is very less. Again, we did optimization
>>   in which the copying from NAND HW buffer to actual buffer is being
>>   done only for those CW's only.
>> 
>>   Again, in this case CPU time will be more.
>> 
> 
> 
> I understand the point and thanks for detailing it. But raw access
> happen either during debug (we don't care about CPU time) or when there
> is an uncorrectable error, which is very unlikely to happen very often
> when using eg. UBI/UBIFS. So I'm still convinced it is better to have a
> _simple_ and straightforward code for this path than something way
> harder to understand and much faster.
> 
> You can add a comment to explain what would be the fastest way and
> why though.
> 

  Thanks Miquel. I will do the changes to make function for
  single codeword raw read.

  Regards,
  Abhishek

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

On 2018-06-07 18:23, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Mon, 28 May 2018 15:40:52 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> On 2018-05-28 12:33, Miquel Raynal wrote:
>> > Hi Abhishek,
>> > >> >>  /* 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)
>> >> >> @@ -2118,6 +2083,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;
>> >> >> >> @@ -2128,11 +2094,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);
>> >> > > Are we sure there is no layout with only 1 step?
>> >> >>   All the layouts are such that, the BBM will come in
>> >>   first byte of spare area.
>> >> >>   For 4 bit ECC, the cw_size is 528 so for 2K page
>> >> >>    2048 - 528 * 3 = 464
>> > > My question was more about small page NANDs. But I suppose it works
>> > too if ecc->steps == 1.
>> >
>>   Correct Miquel.
>> 


>> >> >>   So for last CW, the 464 is BBM (i.e 2048th byte) in
>> >>   full page.
>> >> >> > >> >>  	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]);
>> >> >> >> @@ -2141,12 +2127,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;
>> >> > > This is suspect as it still points to the beginning of the data buffer.
>> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]?
>> >> >
>> >>   The main thing here is
>> >>   nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);
>> >> >>   After reading one complete CW from NAND, the data will be still
>> >>   in NAND HW buffer.
>> >> >>   The above register tells that we need to read data from
>> >>   bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
>> >>   and 2 byte for 16 bus width) in bbm_bytes_buf.
>> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then
>> > it's ok.
>> > >> >>   So bbm_bytes_buf[0] will contain the BBM first byte.
>> >>   and bbm_bytes_buf[1] will contain the BBM second byte.
>> >> >>   Regards,
>> >>   Abhishek
>> >> >> >> >>  	if (chip->options & NAND_BUSWIDTH_16)
>> >> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
>> >> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
>> > > Sorry, my mistake, I did not see the above line.
>> > > However, technically, the BBM could be located in the first, second or
>> > last page of the block. You should check the three of them are 0xFF
>> > before declaring the block is not bad.
>> > > The more I look at the function, the more I wonder if you actually need
>> > it. Why does the generic nand_block_bad() implementation in the core
>> > do not fit?
>> 
>>   The BBM bytes can be accessed in raw mode only for QCOM NAND
>>   Contoller. We started with following patch for initial patches
>> 
>>   http://patchwork.ozlabs.org/patch/508565/
>> 
>>   I am also not very much sure, how can we go ahead now.
>>   Ideally we need to use generic function only which
>>   requires raw_read.
>> 
> 
> I see, thanks for pointing this thread.
> 
> Well for now then let's keep our driver-specific implementation.
> 
> I will just ask you to do a consistent check as requested above (you
> can copy code from the core) and add a comment above this function
> explaining why it is needed (what you just told me).
> 

  Hi Miquel,

  I explored more regarding making custom bad block functions in this
  thread and it looks like, we can move to generic block_bad function
  by small changes in QCOM NAND driver
  only. The main problem was, in read page with ECC, the bad block
  byte was skipped.

  But controller is copying the bad block bytes in another register
  with following status bytes.

  BAD_BLOCK_STATUS : With every page read operation, when the controller
  reads a page with a bad block, it writes the bad block status data into
  this register.

  We can update the BBM bytes at start of OOB data in read_oob function
  with these status bytes. It will help in getting rid of driver-specific
  implementation for chip->block_bad.

  For chip->block_markbad, if we want to get rid of
  driver-specific implementation then we can have
  following logic

  in write_oob function check for bad block bytes in oob
  and do the raw write for updating BBM bytes alone in
  flash if BBM bytes are non 0xff.

  Thanks,
  Abhishek

Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Miquel Raynal]

Hi Abhishek,

On Mon, 11 Jun 2018 14:46:00 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> On 2018-06-07 18:07, Miquel Raynal wrote:
> > Hi Abhishek,  
> > > On Mon, 28 May 2018 11:16:29 +0530, Abhishek Sahu  
> > <absahu@codeaurora.org> wrote:  
> > >> On 2018-05-26 14:12, Miquel Raynal wrote:
> >> > Hi Abhishek,  
> >> > > On Fri, 25 May 2018 17:51:29 +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.  
> >> >> >> This patch introduces nand_ecc_choose_conf 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 v2:  
> >> >> >>   1. Renamed function to nand_ecc_choose_conf.  
> >> >>   2. Minor code reorganization to remove warning and 2 function calls
> >> >>      for nand_maximize_ecc.  
> >> >> >> * Changes from v1:  
> >> >>   NEW PATCH  
> >> >> >>  drivers/mtd/nand/raw/nand_base.c | 42 >> ++++++++++++++++++++++++++++++++++++++++  
> >> >>  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
> >> >>  include/linux/mtd/rawnand.h      |  3 +++
> >> >>  2 files changed, 34 insertions(+)  
> >> >> >> diff --git a/drivers/mtd/nand/raw/nand_base.c >> b/drivers/mtd/nand/raw/nand_base.c  
> >> >> index 72f3a89..e52019d 100644
> >> >> --- a/drivers/mtd/nand/raw/nand_base.c
> >> >> +++ b/drivers/mtd/nand/raw/nand_base.c
> >> >> @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
> >> >>  }
> >> >>  EXPORT_SYMBOL_GPL(nand_maximize_ecc);  
> >> >> >> +/**  
> >> >> + * nand_ecc_choose_conf - 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 configuration 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.
> >> >> + *
> >> >> + * On success, the chosen ECC settings are set.
> >> >> + */
> >> >> +int nand_ecc_choose_conf(struct nand_chip *chip,
> >> >> +			 const struct nand_ecc_caps *caps, int oobavail)
> >> >> +{
> >> >> +	if (chip->ecc.size && chip->ecc.strength)
> >> >> +		return nand_check_ecc_caps(chip, caps, oobavail);
> >> >> +
> >> >> +	if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
> >> >> +	    !nand_match_ecc_req(chip, caps, oobavail))
> >> >> +		return 0;
> >> >> +
> >> >> +	return nand_maximize_ecc(chip, caps, oobavail);
> >> > > I personally don't mind if nand_maximize_ecc() is called twice in  
> >> > the function if it clarifies the logic. Maybe the following will be
> >> > more clear for the user?  
> >> >>   Thanks Miquel.  
> >>   Both the implementations are fine.
> >>   The above implementation (which was in Denali NAND driver) code was >> also
> >>   clear. We can go for any of these implementation.  
> >> >>   Shall I update this ?  
> > > Yes, please :)  
> >   
>   Thanks Miquel for confirming.
>   I will update accordingly.
> 
>   Also, one more question.
> 
>   Shall I make other functions (nand_check_ecc_caps, nand_maximize_ecc
>   and nand_match_ecc_req) static.  Since currently,
>   Denali NAND driver was only using these functions.
> 
>   And Now, this nand_ecc_choose_conf will be help
>   in all the cases.
> 
>   For nand_check_ecc_caps: call nand_ecc_choose_conf with
>                             chip->ecc.size && chip->ecc.strength
> 
>   For nand_maximize_ecc: call nand_ecc_choose_conf with
>                          NAND_ECC_MAXIMIZE
> 
>   For other cases, nand_match_ecc_req will be called.
> 
>   So we will have one external function which will be
>   easy to maintain in future.

If nobody else uses these function outside of the core, then the
symbols should not be exported, their prototypes not in rawnand.h and
they should be declared static.


Regards,
Miquèl

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Miquel Raynal]

Hi Abhishek,

Boris, one question for you below :)

> >> >> >>   So for last CW, the 464 is BBM (i.e 2048th byte) in  
> >> >>   full page.  
> >> >> >> > >> >>  	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]);
> >> >> >> >> @@ -2141,12 +2127,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;
> >> >> > > This is suspect as it still points to the beginning of the data buffer.  
> >> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]?
> >> >> >  
> >> >>   The main thing here is
> >> >>   nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);  
> >> >> >>   After reading one complete CW from NAND, the data will be still  
> >> >>   in NAND HW buffer.  
> >> >> >>   The above register tells that we need to read data from  
> >> >>   bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
> >> >>   and 2 byte for 16 bus width) in bbm_bytes_buf.
> >> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then  
> >> > it's ok.  
> >> > >> >>   So bbm_bytes_buf[0] will contain the BBM first byte.  
> >> >>   and bbm_bytes_buf[1] will contain the BBM second byte.  
> >> >> >>   Regards,  
> >> >>   Abhishek  
> >> >> >> >> >>  	if (chip->options & NAND_BUSWIDTH_16)  
> >> >> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
> >> >> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);  
> >> > > Sorry, my mistake, I did not see the above line.
> >> > > However, technically, the BBM could be located in the first, second or  
> >> > last page of the block. You should check the three of them are 0xFF
> >> > before declaring the block is not bad.  
> >> > > The more I look at the function, the more I wonder if you actually need  
> >> > it. Why does the generic nand_block_bad() implementation in the core
> >> > do not fit?  
> >> >>   The BBM bytes can be accessed in raw mode only for QCOM NAND  
> >>   Contoller. We started with following patch for initial patches  
> >> >>   http://patchwork.ozlabs.org/patch/508565/
> >> >>   I am also not very much sure, how can we go ahead now.  
> >>   Ideally we need to use generic function only which
> >>   requires raw_read.  
> >> > > I see, thanks for pointing this thread.  
> > > Well for now then let's keep our driver-specific implementation.
> > > I will just ask you to do a consistent check as requested above (you  
> > can copy code from the core) and add a comment above this function
> > explaining why it is needed (what you just told me).
> >   
>   Hi Miquel,
> 
>   I explored more regarding making custom bad block functions in this
>   thread and it looks like, we can move to generic block_bad function
>   by small changes in QCOM NAND driver
>   only. The main problem was, in read page with ECC, the bad block
>   byte was skipped.
> 
>   But controller is copying the bad block bytes in another register
>   with following status bytes.
> 
>   BAD_BLOCK_STATUS : With every page read operation, when the controller
>   reads a page with a bad block, it writes the bad block status data into
>   this register.
> 
>   We can update the BBM bytes at start of OOB data in read_oob function
>   with these status bytes. It will help in getting rid of driver-specific
>   implementation for chip->block_bad.

If think this is acceptable.

> 
>   For chip->block_markbad, if we want to get rid of
>   driver-specific implementation then we can have
>   following logic
> 
>   in write_oob function check for bad block bytes in oob
>   and do the raw write for updating BBM bytes alone in
>   flash if BBM bytes are non 0xff.

Ok but this will have to be properly explained in a descriptive comment!

Maybe Boris can give its point of view on the subject. Is it worth
adding the above 'hacks' in the qcom driver and get rid of the
driver-specific ->is_bad()/->mark_bad() impementations?

Thanks,
Miquèl

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

On 2018-06-18 17:05, Miquel Raynal wrote:
> Hi Abhishek,
> 
> Boris, one question for you below :)
> 
>> >> >> >>   So for last CW, the 464 is BBM (i.e 2048th byte) in
>> >> >>   full page.
>> >> >> >> > >> >>  	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]);
>> >> >> >> >> @@ -2141,12 +2127,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;
>> >> >> > > This is suspect as it still points to the beginning of the data buffer.
>> >> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]?
>> >> >> >
>> >> >>   The main thing here is
>> >> >>   nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);
>> >> >> >>   After reading one complete CW from NAND, the data will be still
>> >> >>   in NAND HW buffer.
>> >> >> >>   The above register tells that we need to read data from
>> >> >>   bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
>> >> >>   and 2 byte for 16 bus width) in bbm_bytes_buf.
>> >> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then
>> >> > it's ok.
>> >> > >> >>   So bbm_bytes_buf[0] will contain the BBM first byte.
>> >> >>   and bbm_bytes_buf[1] will contain the BBM second byte.
>> >> >> >>   Regards,
>> >> >>   Abhishek
>> >> >> >> >> >>  	if (chip->options & NAND_BUSWIDTH_16)
>> >> >> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
>> >> >> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
>> >> > > Sorry, my mistake, I did not see the above line.
>> >> > > However, technically, the BBM could be located in the first, second or
>> >> > last page of the block. You should check the three of them are 0xFF
>> >> > before declaring the block is not bad.
>> >> > > The more I look at the function, the more I wonder if you actually need
>> >> > it. Why does the generic nand_block_bad() implementation in the core
>> >> > do not fit?
>> >> >>   The BBM bytes can be accessed in raw mode only for QCOM NAND
>> >>   Contoller. We started with following patch for initial patches
>> >> >>   http://patchwork.ozlabs.org/patch/508565/
>> >> >>   I am also not very much sure, how can we go ahead now.
>> >>   Ideally we need to use generic function only which
>> >>   requires raw_read.
>> >> > > I see, thanks for pointing this thread.
>> > > Well for now then let's keep our driver-specific implementation.
>> > > I will just ask you to do a consistent check as requested above (you
>> > can copy code from the core) and add a comment above this function
>> > explaining why it is needed (what you just told me).
>> >
>>   Hi Miquel,
>> 
>>   I explored more regarding making custom bad block functions in this
>>   thread and it looks like, we can move to generic block_bad function
>>   by small changes in QCOM NAND driver
>>   only. The main problem was, in read page with ECC, the bad block
>>   byte was skipped.
>> 
>>   But controller is copying the bad block bytes in another register
>>   with following status bytes.
>> 
>>   BAD_BLOCK_STATUS : With every page read operation, when the 
>> controller
>>   reads a page with a bad block, it writes the bad block status data 
>> into
>>   this register.
>> 
>>   We can update the BBM bytes at start of OOB data in read_oob 
>> function
>>   with these status bytes. It will help in getting rid of 
>> driver-specific
>>   implementation for chip->block_bad.
> 
> If think this is acceptable.
> 
>> 
>>   For chip->block_markbad, if we want to get rid of
>>   driver-specific implementation then we can have
>>   following logic
>> 
>>   in write_oob function check for bad block bytes in oob
>>   and do the raw write for updating BBM bytes alone in
>>   flash if BBM bytes are non 0xff.
> 
> Ok but this will have to be properly explained in a descriptive 
> comment!
> 
> Maybe Boris can give its point of view on the subject. Is it worth
> adding the above 'hacks' in the qcom driver and get rid of the
> driver-specific ->is_bad()/->mark_bad() impementations?

  Thanks Miquel.

  I will remove this patch from this patch series and will send
  separate patch series for all bad block handling related changes
  once things are finalized.

  Regards,
  Abhishek

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

Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check

[Abhishek Sahu]

On 2018-06-18 17:05, Miquel Raynal wrote:
> Hi Abhishek,
> 
> Boris, one question for you below :)
> 
>> >> >> >>   So for last CW, the 464 is BBM (i.e 2048th byte) in
>> >> >>   full page.
>> >> >> >> > >> >>  	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]);
>> >> >> >> >> @@ -2141,12 +2127,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;
>> >> >> > > This is suspect as it still points to the beginning of the data buffer.
>> >> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]?
>> >> >> >
>> >> >>   The main thing here is
>> >> >>   nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1);
>> >> >> >>   After reading one complete CW from NAND, the data will be still
>> >> >>   in NAND HW buffer.
>> >> >> >>   The above register tells that we need to read data from
>> >> >>   bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh
>> >> >>   and 2 byte for 16 bus width) in bbm_bytes_buf.
>> >> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then
>> >> > it's ok.
>> >> > >> >>   So bbm_bytes_buf[0] will contain the BBM first byte.
>> >> >>   and bbm_bytes_buf[1] will contain the BBM second byte.
>> >> >> >>   Regards,
>> >> >>   Abhishek
>> >> >> >> >> >>  	if (chip->options & NAND_BUSWIDTH_16)
>> >> >> >> -		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
>> >> >> >> +		bad = bad || (bbm_bytes_buf[1] != 0xff);
>> >> > > Sorry, my mistake, I did not see the above line.
>> >> > > However, technically, the BBM could be located in the first, second or
>> >> > last page of the block. You should check the three of them are 0xFF
>> >> > before declaring the block is not bad.
>> >> > > The more I look at the function, the more I wonder if you actually need
>> >> > it. Why does the generic nand_block_bad() implementation in the core
>> >> > do not fit?
>> >> >>   The BBM bytes can be accessed in raw mode only for QCOM NAND
>> >>   Contoller. We started with following patch for initial patches
>> >> >>   http://patchwork.ozlabs.org/patch/508565/
>> >> >>   I am also not very much sure, how can we go ahead now.
>> >>   Ideally we need to use generic function only which
>> >>   requires raw_read.
>> >> > > I see, thanks for pointing this thread.
>> > > Well for now then let's keep our driver-specific implementation.
>> > > I will just ask you to do a consistent check as requested above (you
>> > can copy code from the core) and add a comment above this function
>> > explaining why it is needed (what you just told me).
>> >
>>   Hi Miquel,
>> 
>>   I explored more regarding making custom bad block functions in this
>>   thread and it looks like, we can move to generic block_bad function
>>   by small changes in QCOM NAND driver
>>   only. The main problem was, in read page with ECC, the bad block
>>   byte was skipped.
>> 
>>   But controller is copying the bad block bytes in another register
>>   with following status bytes.
>> 
>>   BAD_BLOCK_STATUS : With every page read operation, when the 
>> controller
>>   reads a page with a bad block, it writes the bad block status data 
>> into
>>   this register.
>> 
>>   We can update the BBM bytes at start of OOB data in read_oob 
>> function
>>   with these status bytes. It will help in getting rid of 
>> driver-specific
>>   implementation for chip->block_bad.
> 
> If think this is acceptable.
> 
>> 
>>   For chip->block_markbad, if we want to get rid of
>>   driver-specific implementation then we can have
>>   following logic
>> 
>>   in write_oob function check for bad block bytes in oob
>>   and do the raw write for updating BBM bytes alone in
>>   flash if BBM bytes are non 0xff.
> 
> Ok but this will have to be properly explained in a descriptive 
> comment!
> 
> Maybe Boris can give its point of view on the subject. Is it worth
> adding the above 'hacks' in the qcom driver and get rid of the
> driver-specific ->is_bad()/->mark_bad() impementations?

  Thanks Miquel.

  I will remove this patch from this patch series and will send
  separate patch series for all bad block handling related changes
  once things are finalized.

  Regards,
  Abhishek