Update for QCOM NAND driver

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 <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(+)

drivers/mtd/nand/raw/nand_base.c +18 -0

index 72f3a89..e52019d 100644
@@ -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,

drivers/mtd/nand/raw/nand_base.c +3 -0

+}
+EXPORT_SYMBOL_GPL(nand_ecc_choose_conf);
+
 /*
  * Check if the chip configuration meet the datasheet requirements.
 

include/linux/mtd/rawnand.h +0 -0

index 5dad59b..89889fa 100644

Hi Abhishek,

Quoted 77 lines · 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 <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(+)

drivers/mtd/nand/raw/nand_base.c +12 -0

index 72f3a89..e52019d 100644
@@ -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.
+ */

drivers/mtd/nand/raw/nand_base.c +7 -0

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

drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6279

Quoted 4 lines

drivers/mtd/nand/raw/nand_base.c +4 -0

+	    !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);

Quoted 12 lines

drivers/mtd/nand/raw/nand_base.c +3 -0

+}
+EXPORT_SYMBOL_GPL(nand_ecc_choose_conf);
+
 /*
  * Check if the chip configuration meet the datasheet requirements.

include/linux/mtd/rawnand.h +0 -0

index 5dad59b..89889fa 100644

include/linux/mtd/rawnand.h int nand_match_ecc_req(struct nand_chip *chip, · line 1635

Quoted 9 lines

include/linux/mtd/rawnand.h +3 -0

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

Quoted 65 lines · Miquel Raynal

Hi Abhishek,

Quoted 1 line

On Fri, 25 May 2018 17:51:29 +0530, Abhishek Sahu

Quoted 62 lines · <[email protected]> wrote:

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

CC Masahiro Yamada <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* Changes from v2:

Quoted 1 line

Quoted 1 line

1. Renamed function to nand_ecc_choose_conf.

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

Quoted 1 line

Quoted 1 line

* Changes from v1:

NEW PATCH

Quoted 1 line

Quoted 1 line

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

Quoted 1 line

drivers/mtd/nand/raw/nand_base.c

Quoted 1 line

drivers/mtd/nand/raw/nand_base.c renamed from drivers/mtd/nand/raw/nand_base.c >> +0 -0

index 72f3a89..e52019d 100644

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
 }
 EXPORT_SYMBOL_GPL(nand_maximize_ecc);  

Quoted 1 line

Quoted 1 line

+/**

+ * 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.
+ */

drivers/mtd/nand/raw/nand_base.c +6 -0

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

Quoted 8 lines

Quoted 6 lines

drivers/mtd/nand/raw/nand_base.c +5 -0

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

Quoted 5 lines

Quoted 2 lines

	if (chip->ecc.size && chip->ecc.strength)  

		return nand_check_ecc_caps(chip, caps, oobavail);  

Quoted 1 line

	if (chip->ecc.options & NAND_ECC_MAXIMIZE)  

		return nand_maximize_ecc(chip, caps, oobavail);  

Quoted 6 lines

Quoted 1 line

if (!nand_match_ecc_req(chip, caps, oobavail))

		return 0;  

Quoted 2 lines

	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. :-)

Hi Abhishek,

Quoted 81 lines · Abhishek Sahu

Quoted 65 lines · Miquel Raynal

Hi Abhishek,

Quoted 1 line

On Fri, 25 May 2018 17:51:29 +0530, Abhishek Sahu

Quoted 62 lines · <[email protected]> wrote:

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

CC Masahiro Yamada <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* Changes from v2:

Quoted 1 line

Quoted 1 line

1. Renamed function to nand_ecc_choose_conf.

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

Quoted 1 line

Quoted 1 line

* Changes from v1:

NEW PATCH

Quoted 1 line

Quoted 1 line

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

Quoted 1 line

drivers/mtd/nand/raw/nand_base.c

Quoted 1 line

drivers/mtd/nand/raw/nand_base.c renamed from drivers/mtd/nand/raw/nand_base.c >> +0 -0

index 72f3a89..e52019d 100644

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
 }
 EXPORT_SYMBOL_GPL(nand_maximize_ecc);

Quoted 1 line

Quoted 1 line

+/**

+ * 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.
+ */

drivers/mtd/nand/raw/nand_base.c +6 -0

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

Quoted 8 lines

Quoted 6 lines

drivers/mtd/nand/raw/nand_base.c +5 -0

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

Quoted 1 line

Shall I update this ?

Yes, please :)

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

	if (chip->ecc.size && chip->ecc.strength)

		return nand_check_ecc_caps(chip, caps, oobavail);

Quoted 1 line

	if (chip->ecc.options & NAND_ECC_MAXIMIZE)

		return nand_maximize_ecc(chip, caps, oobavail);

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

if (!nand_match_ecc_req(chip, caps, oobavail))

		return 0;

Quoted 2 lines

	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

Quoted 84 lines · Miquel Raynal

Hi Abhishek,

Quoted 1 line

On Mon, 28 May 2018 11:16:29 +0530, Abhishek Sahu

Quoted 80 lines · <[email protected]> wrote:

Quoted 66 lines · Miquel Raynal

Hi Abhishek,

Quoted 1 line

On Fri, 25 May 2018 17:51:29 +0530, Abhishek Sahu

Quoted 62 lines · <[email protected]> wrote:

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

CC Masahiro Yamada <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* Changes from v2:

Quoted 1 line

Quoted 1 line

1. Renamed function to nand_ecc_choose_conf.

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

Quoted 1 line

Quoted 1 line

* Changes from v1:

NEW PATCH

Quoted 1 line

Quoted 1 line

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

Quoted 1 line

drivers/mtd/nand/raw/nand_base.c

Quoted 1 line

drivers/mtd/nand/raw/nand_base.c renamed from drivers/mtd/nand/raw/nand_base.c >> +0 -0

index 72f3a89..e52019d 100644

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
 }
 EXPORT_SYMBOL_GPL(nand_maximize_ecc);  

Quoted 1 line

Quoted 1 line

+/**

+ * 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.
+ */

drivers/mtd/nand/raw/nand_base.c +6 -0

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

Quoted 9 lines

Quoted 6 lines

drivers/mtd/nand/raw/nand_base.c +5 -0

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

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

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.

Hi Abhishek,

Quoted 66 lines · 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 <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(+)

drivers/mtd/nand/raw/nand_base.c +18 -0

index 72f3a89..e52019d 100644
@@ -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,

drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6279

Quoted 10 lines

drivers/mtd/nand/raw/nand_base.c +10 -0

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

Quoted 86 lines · On Sat, 26 May 2018 10:42:47 +0200 Miquel Raynal <[email protected]>…

Hi Abhishek,

Quoted 66 lines · 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 <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(+)

drivers/mtd/nand/raw/nand_base.c +18 -0

index 72f3a89..e52019d 100644
@@ -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,

drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6279

Quoted 10 lines

drivers/mtd/nand/raw/nand_base.c +10 -0

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

Quoted 10 lines

      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.

Quoted 10 lines

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.

Quoted 13 lines

              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;

Quoted 3 lines

maximize_ecc:

      return nand_maximize_ecc(chip, caps, oobavail);

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

Hi.

2018-05-30 4:30 GMT+09:00 Boris Brezillon <[email protected]>:

Quoted 132 lines

Quoted 86 lines · On Sat, 26 May 2018 10:42:47 +0200 Miquel Raynal <[email protected]>…

Hi Abhishek,

Quoted 66 lines · 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 <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(+)

drivers/mtd/nand/raw/nand_base.c +18 -0

index 72f3a89..e52019d 100644
@@ -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,

drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6279

Quoted 10 lines

drivers/mtd/nand/raw/nand_base.c +10 -0

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

Quoted 10 lines

      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.

Quoted 10 lines

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.

Quoted 13 lines

              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;

Quoted 8 lines

Quoted 3 lines

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.

Quoted 185 lines

Hi.

2018-05-30 4:30 GMT+09:00 Boris Brezillon <[email protected]>:

Quoted 141 lines

Quoted 92 lines · On Sat, 26 May 2018 10:42:47 +0200 Miquel Raynal <[email protected]>…

Hi Abhishek,

Quoted 72 lines · 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 <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(+)

drivers/mtd/nand/raw/nand_base.c +12 -0

index 72f3a89..e52019d 100644
@@ -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.
+ */

drivers/mtd/nand/raw/nand_base.c +1 -0

+int nand_ecc_choose_conf(struct nand_chip *chip,

drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6279

Quoted 10 lines

drivers/mtd/nand/raw/nand_base.c +10 -0

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

Quoted 10 lines

      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.

Quoted 10 lines

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.

Quoted 13 lines

              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;

Quoted 8 lines

Quoted 3 lines

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);
}

Quoted 5 lines

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

Quoted 4 lines

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/

2018-05-30 15:21 GMT+09:00 Abhishek Sahu <[email protected]>:

Quoted 193 lines · Masahiro Yamada

Quoted 186 lines

Hi.

2018-05-30 4:30 GMT+09:00 Boris Brezillon <[email protected]>:

Quoted 141 lines

Quoted 92 lines · On Sat, 26 May 2018 10:42:47 +0200 Miquel Raynal <[email protected]>…

Hi Abhishek,

Quoted 72 lines · 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 <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(+)

drivers/mtd/nand/raw/nand_base.c +12 -0

index 72f3a89..e52019d 100644
@@ -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.
+ */

drivers/mtd/nand/raw/nand_base.c +1 -0

+int nand_ecc_choose_conf(struct nand_chip *chip,

drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6279

Quoted 10 lines

drivers/mtd/nand/raw/nand_base.c +10 -0

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

Quoted 10 lines

      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.

Quoted 10 lines

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.

Quoted 13 lines

              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;

Quoted 8 lines

Quoted 3 lines

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);
}

Quoted 5 lines

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

Quoted 6 lines

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().

Quoted 10 lines

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.

Quoted 12 lines

Thanks,
Abhishek

Quoted 4 lines

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/

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

CC Masahiro Yamada <[email protected]>

Acked-by Miquel Raynal <[email protected]>

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(-)

drivers/mtd/nand/raw/denali.c +2 -29

index 2a302a1..a586a1d 100644
@@ -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/

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 <[email protected]>

---
* Changes from v2:
NONE

* Changes from v1:
NEW PATCH

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

Documentation/devicetree/bindings/mtd/qcom_nandc.txt +0 -0

index 73d336be..f246aa0 100644

Documentation/devicetree/bindings/mtd/qcom_nandc.txt +3 -0

 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

Hi Abhishek,

Quoted 15 lines · 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 <[email protected]>

---
* Changes from v2:
NONE

* Changes from v1:
NEW PATCH

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

Documentation/devicetree/bindings/mtd/qcom_nandc.txt Required properties: · line 48

Quoted 10 lines

Documentation/devicetree/bindings/mtd/qcom_nandc.txt +0 -1

index 73d336be..f246aa0 100644
@@ -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.

Quoted 12 lines

Documentation/devicetree/bindings/mtd/qcom_nandc.txt +1 -0

 Optional properties:
 - nand-bus-width:	see nand.txt
+- nand-ecc-strength:	see nand.txt. If not specified, then ECC 

strength will

Documentation/devicetree/bindings/mtd/qcom_nandc.txt +2 -0

+			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

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 <[email protected]>

---
* 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(-)

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

index b554fb6..b538390 100644
@@ -2325,15 +2325,8 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
 	bool wide_bus;
 	int ecc_mode = 1;
 

Hi Abhishek,

Quoted 23 lines · 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 <[email protected]>

---
* 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(-)

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

index b554fb6..b538390 100644
@@ -2325,15 +2325,8 @@ static int qcom_nand_host_setup(struct 
 qcom_nand_host *host)
 	bool wide_bus;
 	int ecc_mode = 1;
 

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nand_host_setup(struct qcom_nand_host *host) · line 2328

Quoted 10 lines

drivers/mtd/nand/raw/qcom_nandc.c +1 -9

-	/*
-	 * 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"

Quoted 4 lines

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

+	ecc->size = NANDC_STEP_SIZE;
 	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
 

if (ecc->strength >= 8) {

Once corrected:

Acked-by Miquel Raynal <[email protected]>

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 <[email protected]>

---
* 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(-)

drivers/mtd/nand/raw/qcom_nandc.c +9 -1

index b538390..7377923 100644
@@ -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 */

drivers/mtd/nand/raw/qcom_nandc.c +4 -1

+	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
 	 */

Hi Abhishek,

Quoted 9 lines · 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 <[email protected]>

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

Quoted 36 lines

* 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(-)

drivers/mtd/nand/raw/qcom_nandc.c +9 -1

index b538390..7377923 100644
@@ -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;
 

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nand_ooblayout_free(struct mtd_info *mtd, int section, · line 2346

Quoted 12 lines

drivers/mtd/nand/raw/qcom_nandc.c +4 -0

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

drivers/mtd/nand/raw/qcom_nandc.c +4 -0

+	 * 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.

Quoted 30 lines

drivers/mtd/nand/raw/qcom_nandc.c +4 -7

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

Quoted 1 line

host->cfg0 = (cwperpage - 1) << CW_PER_PAGE

Once corrected:

Acked-by Miquel Raynal <[email protected]>

Thanks,
Miquèl

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 [email protected]

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(-)

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

Signed-off-by Abhishek Sahu <[email protected]>

---

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 <[email protected]>

---
* 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(-)

drivers/mtd/nand/raw/qcom_nandc.c +1 -35

index d693b5f..f72bc8a 100644
@@ -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;
 

drivers/mtd/nand/raw/qcom_nandc.c +21 -2

 	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]);
 

Hi Abhishek,

Quoted 2 lines · 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.

"is the first spare byte"?

Quoted 2 lines

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

"It does the following:"

Quoted 9 lines

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.

Quoted 2 lines

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

Quoted 71 lines

Signed-off-by Abhishek Sahu <[email protected]>

---
* 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(-)

drivers/mtd/nand/raw/qcom_nandc.c +0 -35

index d693b5f..f72bc8a 100644
@@ -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,

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 				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;

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_block_bad(struct · line 2098

Quoted 6 lines

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

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

Quoted 35 lines

drivers/mtd/nand/raw/qcom_nandc.c +21 -2

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

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) · line 2130

Quoted 4 lines

drivers/mtd/nand/raw/qcom_nandc.c +1 -3

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

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) · line 2136

Quoted 7 lines

drivers/mtd/nand/raw/qcom_nandc.c +1 -1

 	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

Quoted 8 lines

Quoted 8 lines

/* implements ecc->read_page() */
static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip,

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 				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;  

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -0

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

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 	 */
 	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.

Quoted 29 lines

Quoted 29 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

clear_bam_transaction(nandc);

drivers/mtd/nand/raw/qcom_nandc.c +21 -2

-	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;
+	}  

Quoted 2 lines

Quoted 2 lines

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 	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)  

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 		goto err;
 	}  

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

drivers/mtd/nand/raw/qcom_nandc.c +0 -1

-	bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1);  

drivers/mtd/nand/raw/qcom_nandc.c +1 -2

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

Quoted 3 lines

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) · line 2133

Quoted 3 lines

Quoted 1 line

Quoted 1 line

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 	if (chip->options & NAND_BUSWIDTH_16)  

drivers/mtd/nand/raw/qcom_nandc.c +1 -1

-		bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
+		bad = bad || (bbm_bytes_buf[1] != 0xff);

Quoted 5 lines

Quoted 4 lines

err:
return bad;
}

Quoted 1 line

Thanks,

Miquèl

Hi Abhishek,

Quoted 19 lines

Quoted 8 lines

Quoted 8 lines

/* implements ecc->read_page() */
static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip,

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 				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;

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -0

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

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 	 */
 	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.

Quoted 3 lines

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.

Quoted 43 lines

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

Quoted 29 lines

Quoted 29 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

clear_bam_transaction(nandc);

drivers/mtd/nand/raw/qcom_nandc.c +21 -2

-	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;
+	}

Quoted 2 lines

Quoted 2 lines

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 	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)

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 		goto err;
 	}

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

drivers/mtd/nand/raw/qcom_nandc.c +0 -1

-	bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1);

drivers/mtd/nand/raw/qcom_nandc.c +1 -2

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

Quoted 6 lines

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.

Quoted 10 lines

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

Regards,
Abhishek

Quoted 3 lines

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) · line 2133

Quoted 3 lines

Quoted 1 line

Quoted 1 line

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 	if (chip->options & NAND_BUSWIDTH_16)

drivers/mtd/nand/raw/qcom_nandc.c +1 -1

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

Quoted 18 lines · Miquel Raynal

Hi Abhishek,

Quoted 17 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

/* implements ecc->read_page() */

static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip,

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 				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;  

Quoted 8 lines

Quoted 8 lines

(quoted diff) +0 -0

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

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 	 */
 	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.

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 56 lines

Quoted 38 lines

Quoted 1 line

Quoted 1 line

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

full page.

Quoted 29 lines

Quoted 29 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

clear_bam_transaction(nandc);

drivers/mtd/nand/raw/qcom_nandc.c +21 -2

-	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;
+	}  

Quoted 2 lines

Quoted 2 lines

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 	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)  

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 		goto err;
 	}  

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

drivers/mtd/nand/raw/qcom_nandc.c +0 -1

-	bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1);  

drivers/mtd/nand/raw/qcom_nandc.c +1 -2

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

Quoted 8 lines

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

Quoted 1 line

Quoted 1 line

After reading one complete CW from NAND, the data will be still

in NAND HW buffer.

Quoted 1 line

Quoted 1 line

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.

Quoted 9 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

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

Quoted 1 line

Quoted 1 line

Regards,

Abhishek

Quoted 3 lines

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) · line 2133

Quoted 3 lines

Quoted 1 line

Quoted 1 line

(quoted diff) +0 -0

 >>  	if (chip->options & NAND_BUSWIDTH_16)  

drivers/mtd/nand/raw/qcom_nandc.c +1 -1

-		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

Quoted 5 lines

last page of the block. You should check the three of them are 0xFF before declaring the block is not bad.

Quoted 1 line

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.

Hi Abhishek,

Quoted 90 lines · Abhishek Sahu

Quoted 18 lines · Miquel Raynal

Hi Abhishek,

Quoted 17 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

/* implements ecc->read_page() */

static int qcom_nandc_read_page(struct mtd_info *mtd, struct >> nand_chip *chip,

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 				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;

Quoted 8 lines

Quoted 8 lines

(quoted diff) +0 -0

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

drivers/mtd/nand/raw/qcom_nandc.c +1 -0

 	 */
 	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.

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 56 lines

Quoted 38 lines

Quoted 1 line

Quoted 1 line

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

full page.

Quoted 29 lines

Quoted 29 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

clear_bam_transaction(nandc);

drivers/mtd/nand/raw/qcom_nandc.c +21 -2

-	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;
+	}

Quoted 2 lines

Quoted 2 lines

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 	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)

drivers/mtd/nand/raw/qcom_nandc.c +0 -0

 		goto err;
 	}

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

drivers/mtd/nand/raw/qcom_nandc.c +0 -1

-	bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1);

drivers/mtd/nand/raw/qcom_nandc.c +1 -2

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

Quoted 8 lines

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

Quoted 1 line

Quoted 1 line

After reading one complete CW from NAND, the data will be still

in NAND HW buffer.

Quoted 1 line

Quoted 1 line

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.

Quoted 9 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

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

Quoted 1 line

Quoted 1 line

Regards,

Abhishek

Quoted 3 lines

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) · line 2133

Quoted 3 lines

Quoted 1 line

Quoted 1 line

(quoted diff) +0 -0

 >>  	if (chip->options & NAND_BUSWIDTH_16)

drivers/mtd/nand/raw/qcom_nandc.c +1 -1

-		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

Quoted 5 lines

last page of the block. You should check the three of them are 0xFF before declaring the block is not bad.

Quoted 1 line

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?

Quoted 9 lines

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

Quoted 65 lines

Quoted 65 lines

Quoted 56 lines

Quoted 38 lines

Quoted 1 line

Quoted 1 line

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

full page.

Quoted 29 lines

Quoted 29 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

clear_bam_transaction(nandc);

(quoted diff) +21 -2

-	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;
+	}  

Quoted 2 lines

Quoted 2 lines

(quoted diff) +0 -0

 	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)  

(quoted diff) +0 -0

 		goto err;
 	}  

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

(quoted diff) +0 -1

-	bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1);  

(quoted diff) +1 -2

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

Quoted 8 lines

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

Quoted 1 line

Quoted 1 line

After reading one complete CW from NAND, the data will be still

in NAND HW buffer.

Quoted 1 line

Quoted 1 line

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.

Quoted 9 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

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

Quoted 1 line

Quoted 1 line

Regards,

Abhishek

Quoted 3 lines

Review

Quoted 3 lines

Quoted 1 line

Quoted 1 line

(quoted diff) +0 -0

 >>  	if (chip->options & NAND_BUSWIDTH_16)  

(quoted diff) +1 -1

-		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

Quoted 6 lines

last page of the block. You should check the three of them are 0xFF before declaring the block is not bad.

Quoted 1 line

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?

Quoted 1 line

The BBM bytes can be accessed in raw mode only for QCOM NAND

Contoller. We started with following patch for initial patches

Quoted 2 lines

Quoted 2 lines

http://patchwork.ozlabs.org/patch/508565/
I am also not very much sure, how can we go ahead now.

Quoted 8 lines

Quoted 5 lines

Ideally we need to use generic function only which requires raw_read.

Quoted 1 line

Quoted 1 line

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.