[PATCH 0/4] rawnand: pl35x: Implement mixed ECC

[PATCH 0/4] rawnand: pl35x: Implement mixed ECC

[Bastien Curutchet]

Hi all,

This series addresses the PL353 errata "SLC ECC misses single error to
bit0 byte0 and fails to detect some double error cases" (see 721059 in
[1]).

The ECC HW engine is known to be broken on this chip, it fails to detect
some bitflips on reads.

This series implements a mixed-ECC where the HW engine is used on writes
and reads are done by software. To make it work, the software ECC has to
use the same layout for the ECC bits than the HW engine. Patches 1 & 2
add the PL35x ordering to the available orderings for soft ECC. Patches
3 & 4 make use of this ordering to implement the mixed-ECC for the PL353
controller.

Note that this series is based on top of nand/next.

[1]: https://developer.arm.com/documentation/rlnc000227/a

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
Bastien Curutchet (4):
      nand: hamming: Replace sm_order boolean with enum
      nand: hamming: Add support for the PL35x ECC bit ordering
      rawnand: base: Export nand_read_page_swecc
      rawnand: pl35x: Implement mixed ECC computing

 drivers/mtd/nand/ecc-sw-hamming.c            | 179 ++++++++++++++++++---------
 drivers/mtd/nand/raw/nand_base.c             |  11 +-
 drivers/mtd/nand/raw/pl35x-nand-controller.c | 173 ++------------------------
 include/linux/mtd/nand-ecc-sw-hamming.h      |  20 ++-
 include/linux/mtd/rawnand.h                  |   7 +-
 5 files changed, 161 insertions(+), 229 deletions(-)
---
base-commit: f97bdc8ec1dc7b33781a702eeba55326c206be56
change-id: 20260529-mix-ecc-46e8ccd4e9ae

Best regards,
-- 
Bastien Curutchet <bastien.curutchet@bootlin.com>


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

[PATCH 1/4] nand: hamming: Replace sm_order boolean with enum

[Bastien Curutchet]

The ECC bit ordering with Hamming code isn't standardized so we
can find several kind of ordering. So far two ordering are handled: the
'regular' one and the 'smart media' one. The 'smart media' ordering is
selected through the sm_order boolean. Using a boolean to choose the bit
ordering prevents from supporting more than two ECC bit orderings.

Create a new enum to represent the supported ECC bit ordering for
Hamming code.
Replace the sm_order boolean with this enum.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/ecc-sw-hamming.c       | 29 ++++++++++++++++++-----------
 drivers/mtd/nand/raw/nand_base.c        |  4 +++-
 include/linux/mtd/nand-ecc-sw-hamming.h | 19 +++++++++++++------
 3 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/ecc-sw-hamming.c b/drivers/mtd/nand/ecc-sw-hamming.c
index 460acc1029c3..d4127e30726e 100644
--- a/drivers/mtd/nand/ecc-sw-hamming.c
+++ b/drivers/mtd/nand/ecc-sw-hamming.c
@@ -113,7 +113,7 @@ static const char addressbits[256] = {
 };
 
 int ecc_sw_hamming_calculate(const unsigned char *buf, unsigned int step_size,
-			     unsigned char *code, bool sm_order)
+			     unsigned char *code, enum ecc_hamming_order ecc_order)
 {
 	const u32 *bp = (uint32_t *)buf;
 	const u32 eccsize_mult = (step_size == 256) ? 1 : 2;
@@ -309,7 +309,8 @@ int ecc_sw_hamming_calculate(const unsigned char *buf, unsigned int step_size,
 	 * possible, but benchmarks showed that on the system this is developed
 	 * the code below is the fastest
 	 */
-	if (sm_order) {
+	switch (ecc_order) {
+	case ECC_HAMMING_SM_ORDER:
 		code[0] = (invparity[rp7] << 7) | (invparity[rp6] << 6) |
 			  (invparity[rp5] << 5) | (invparity[rp4] << 4) |
 			  (invparity[rp3] << 3) | (invparity[rp2] << 2) |
@@ -318,7 +319,8 @@ int ecc_sw_hamming_calculate(const unsigned char *buf, unsigned int step_size,
 			  (invparity[rp13] << 5) | (invparity[rp12] << 4) |
 			  (invparity[rp11] << 3) | (invparity[rp10] << 2) |
 			  (invparity[rp9] << 1) | (invparity[rp8]);
-	} else {
+		break;
+	case ECC_HAMMING_REGULAR_ORDER:
 		code[1] = (invparity[rp7] << 7) | (invparity[rp6] << 6) |
 			  (invparity[rp5] << 5) | (invparity[rp4] << 4) |
 			  (invparity[rp3] << 3) | (invparity[rp2] << 2) |
@@ -327,6 +329,7 @@ int ecc_sw_hamming_calculate(const unsigned char *buf, unsigned int step_size,
 			  (invparity[rp13] << 5) | (invparity[rp12] << 4) |
 			  (invparity[rp11] << 3) | (invparity[rp10] << 2) |
 			  (invparity[rp9] << 1) | (invparity[rp8]);
+		break;
 	}
 
 	if (eccsize_mult == 1)
@@ -364,15 +367,16 @@ int nand_ecc_sw_hamming_calculate(struct nand_device *nand,
 {
 	struct nand_ecc_sw_hamming_conf *engine_conf = nand->ecc.ctx.priv;
 	unsigned int step_size = nand->ecc.ctx.conf.step_size;
-	bool sm_order = engine_conf ? engine_conf->sm_order : false;
+	enum ecc_hamming_order order = engine_conf ? engine_conf->ecc_order :
+				       ECC_HAMMING_REGULAR_ORDER;
 
-	return ecc_sw_hamming_calculate(buf, step_size, code, sm_order);
+	return ecc_sw_hamming_calculate(buf, step_size, code, order);
 }
 EXPORT_SYMBOL(nand_ecc_sw_hamming_calculate);
 
 int ecc_sw_hamming_correct(unsigned char *buf, unsigned char *read_ecc,
 			   unsigned char *calc_ecc, unsigned int step_size,
-			   bool sm_order)
+			   enum ecc_hamming_order ecc_order)
 {
 	const u32 eccsize_mult = step_size >> 8;
 	unsigned char b0, b1, b2, bit_addr;
@@ -383,14 +387,16 @@ int ecc_sw_hamming_correct(unsigned char *buf, unsigned char *read_ecc,
 	 * we might need the xor result  more than once,
 	 * so keep them in a local var
 	*/
-	if (sm_order) {
+	switch (ecc_order) {
+	case ECC_HAMMING_SM_ORDER:
 		b0 = read_ecc[0] ^ calc_ecc[0];
 		b1 = read_ecc[1] ^ calc_ecc[1];
-	} else {
+		break;
+	case ECC_HAMMING_REGULAR_ORDER:
 		b0 = read_ecc[1] ^ calc_ecc[1];
 		b1 = read_ecc[0] ^ calc_ecc[0];
+		break;
 	}
-
 	b2 = read_ecc[2] ^ calc_ecc[2];
 
 	/* check if there are any bitfaults */
@@ -457,10 +463,11 @@ int nand_ecc_sw_hamming_correct(struct nand_device *nand, unsigned char *buf,
 {
 	struct nand_ecc_sw_hamming_conf *engine_conf = nand->ecc.ctx.priv;
 	unsigned int step_size = nand->ecc.ctx.conf.step_size;
-	bool sm_order = engine_conf ? engine_conf->sm_order : false;
+	enum ecc_hamming_order ecc_order = engine_conf ? engine_conf->ecc_order :
+					   ECC_HAMMING_REGULAR_ORDER;
 
 	return ecc_sw_hamming_correct(buf, read_ecc, calc_ecc, step_size,
-				      sm_order);
+				      ecc_order);
 }
 EXPORT_SYMBOL(nand_ecc_sw_hamming_correct);
 
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index a5b278ab9384..f6b6a4fa2b1f 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5657,7 +5657,9 @@ int rawnand_sw_hamming_init(struct nand_chip *chip)
 	engine_conf = base->ecc.ctx.priv;
 
 	if (chip->ecc.options & NAND_ECC_SOFT_HAMMING_SM_ORDER)
-		engine_conf->sm_order = true;
+		engine_conf->ecc_order = ECC_HAMMING_SM_ORDER;
+	else
+		engine_conf->ecc_order = ECC_HAMMING_REGULAR_ORDER;
 
 	chip->ecc.size = base->ecc.ctx.conf.step_size;
 	chip->ecc.strength = base->ecc.ctx.conf.strength;
diff --git a/include/linux/mtd/nand-ecc-sw-hamming.h b/include/linux/mtd/nand-ecc-sw-hamming.h
index 2aa2f8ef68d2..ac6f28f35f77 100644
--- a/include/linux/mtd/nand-ecc-sw-hamming.h
+++ b/include/linux/mtd/nand-ecc-sw-hamming.h
@@ -12,6 +12,11 @@
 
 #include <linux/mtd/nand.h>
 
+enum ecc_hamming_order {
+	ECC_HAMMING_REGULAR_ORDER = 0,
+	ECC_HAMMING_SM_ORDER,
+};
+
 /**
  * struct nand_ecc_sw_hamming_conf - private software Hamming ECC engine structure
  * @req_ctx: Save request context and tweak the original request to fit the
@@ -19,14 +24,14 @@
  * @code_size: Number of bytes needed to store a code (one code per step)
  * @calc_buf: Buffer to use when calculating ECC bytes
  * @code_buf: Buffer to use when reading (raw) ECC bytes from the chip
- * @sm_order: Smart Media special ordering
+ * @ecc_order: ECC ordering
  */
 struct nand_ecc_sw_hamming_conf {
 	struct nand_ecc_req_tweak_ctx req_ctx;
 	unsigned int code_size;
 	u8 *calc_buf;
 	u8 *code_buf;
-	unsigned int sm_order;
+	enum ecc_hamming_order ecc_order;
 };
 
 #if IS_ENABLED(CONFIG_MTD_NAND_ECC_SW_HAMMING)
@@ -34,13 +39,13 @@ struct nand_ecc_sw_hamming_conf {
 int nand_ecc_sw_hamming_init_ctx(struct nand_device *nand);
 void nand_ecc_sw_hamming_cleanup_ctx(struct nand_device *nand);
 int ecc_sw_hamming_calculate(const unsigned char *buf, unsigned int step_size,
-			     unsigned char *code, bool sm_order);
+			     unsigned char *code, enum ecc_hamming_order ecc_order);
 int nand_ecc_sw_hamming_calculate(struct nand_device *nand,
 				  const unsigned char *buf,
 				  unsigned char *code);
 int ecc_sw_hamming_correct(unsigned char *buf, unsigned char *read_ecc,
 			   unsigned char *calc_ecc, unsigned int step_size,
-			   bool sm_order);
+			   enum ecc_hamming_order ecc_order);
 int nand_ecc_sw_hamming_correct(struct nand_device *nand, unsigned char *buf,
 				unsigned char *read_ecc,
 				unsigned char *calc_ecc);
@@ -56,7 +61,8 @@ static inline void nand_ecc_sw_hamming_cleanup_ctx(struct nand_device *nand) {}
 
 static inline int ecc_sw_hamming_calculate(const unsigned char *buf,
 					   unsigned int step_size,
-					   unsigned char *code, bool sm_order)
+					   unsigned char *code,
+					   enum ecc_hamming_order ecc_order)
 {
 	return -ENOTSUPP;
 }
@@ -71,7 +77,8 @@ static inline int nand_ecc_sw_hamming_calculate(struct nand_device *nand,
 static inline int ecc_sw_hamming_correct(unsigned char *buf,
 					 unsigned char *read_ecc,
 					 unsigned char *calc_ecc,
-					 unsigned int step_size, bool sm_order)
+					 unsigned int step_size,
+					 enum ecc_hamming_order ecc_order)
 {
 	return -ENOTSUPP;
 }

-- 
2.55.0


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

[PATCH 2/4] nand: hamming: Add support for the PL35x ECC bit ordering

[Bastien Curutchet]

The PL353's HW-ECC engine is broken and fails to report some errors
on reading (cf errata 721059 in [1]). So, to still benefit a bit from the
HW acceleration, a mixed approach has to be used: where writes are
performed by the HW-ECC engine and reads by software. To do so, the
software part has to use the same bit ordering than the hardware engine.

Add support for the PL35x bit ordering in the generic hamming helpers.
Create a new option flag to enable this bit ordering. For consistency
this new flag is put next to the SM_ORDER flag which shifts the CACHEPRG
one bit away.

[1] : https://developer.arm.com/documentation/rlnc000227/a
Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/ecc-sw-hamming.c       | 150 ++++++++++++++++++++++----------
 drivers/mtd/nand/raw/nand_base.c        |   2 +
 include/linux/mtd/nand-ecc-sw-hamming.h |   1 +
 include/linux/mtd/rawnand.h             |   5 +-
 4 files changed, 111 insertions(+), 47 deletions(-)

diff --git a/drivers/mtd/nand/ecc-sw-hamming.c b/drivers/mtd/nand/ecc-sw-hamming.c
index d4127e30726e..40f453299c7a 100644
--- a/drivers/mtd/nand/ecc-sw-hamming.c
+++ b/drivers/mtd/nand/ecc-sw-hamming.c
@@ -320,6 +320,24 @@ int ecc_sw_hamming_calculate(const unsigned char *buf, unsigned int step_size,
 			  (invparity[rp11] << 3) | (invparity[rp10] << 2) |
 			  (invparity[rp9] << 1) | (invparity[rp8]);
 		break;
+	case ECC_HAMMING_PL35X_ORDER:
+		code[1] = (invparity[rp0] << 7) |
+			  (invparity[par & 0x0f] << 6) |
+			  (invparity[par & 0x33] << 5) |
+			  (invparity[par & 0x55] << 4) |
+			  (invparity[rp17] << 3) |
+			  (invparity[rp15] << 2) |
+			  (invparity[rp13] << 1) |
+			  (invparity[rp11] << 0);
+		code[0] = (invparity[rp9] << 7) |
+			  (invparity[rp7] << 6) |
+			  (invparity[rp5] << 5) |
+			  (invparity[rp3] << 4) |
+			  (invparity[rp1] << 3) |
+			  (invparity[par & 0xf0] << 2) |
+			  (invparity[par & 0xcc] << 1) |
+			  (invparity[par & 0xaa] << 0);
+		break;
 	case ECC_HAMMING_REGULAR_ORDER:
 		code[1] = (invparity[rp7] << 7) | (invparity[rp6] << 6) |
 			  (invparity[rp5] << 5) | (invparity[rp4] << 4) |
@@ -332,25 +350,38 @@ int ecc_sw_hamming_calculate(const unsigned char *buf, unsigned int step_size,
 		break;
 	}
 
-	if (eccsize_mult == 1)
-		code[2] =
-		    (invparity[par & 0xf0] << 7) |
-		    (invparity[par & 0x0f] << 6) |
-		    (invparity[par & 0xcc] << 5) |
-		    (invparity[par & 0x33] << 4) |
-		    (invparity[par & 0xaa] << 3) |
-		    (invparity[par & 0x55] << 2) |
-		    3;
-	else
-		code[2] =
-		    (invparity[par & 0xf0] << 7) |
-		    (invparity[par & 0x0f] << 6) |
-		    (invparity[par & 0xcc] << 5) |
-		    (invparity[par & 0x33] << 4) |
-		    (invparity[par & 0xaa] << 3) |
-		    (invparity[par & 0x55] << 2) |
-		    (invparity[rp17] << 1) |
-		    (invparity[rp16] << 0);
+	switch (ecc_order) {
+	case ECC_HAMMING_PL35X_ORDER:
+		code[2] = (invparity[rp16] << 7) |
+			  (invparity[rp14] << 6) |
+			  (invparity[rp12] << 5) |
+			  (invparity[rp10] << 4) |
+			  (invparity[rp8] << 3) |
+			  (invparity[rp6] << 2) |
+			  (invparity[rp4] << 1) |
+			  (invparity[rp2] << 0);
+		break;
+	case ECC_HAMMING_REGULAR_ORDER:
+	case ECC_HAMMING_SM_ORDER:
+		if (eccsize_mult == 1)
+			code[2] = (invparity[par & 0xf0] << 7) |
+				  (invparity[par & 0x0f] << 6) |
+				  (invparity[par & 0xcc] << 5) |
+				  (invparity[par & 0x33] << 4) |
+				  (invparity[par & 0xaa] << 3) |
+				  (invparity[par & 0x55] << 2) |
+				  3;
+		else
+			code[2] = (invparity[par & 0xf0] << 7) |
+				  (invparity[par & 0x0f] << 6) |
+				  (invparity[par & 0xcc] << 5) |
+				  (invparity[par & 0x33] << 4) |
+				  (invparity[par & 0xaa] << 3) |
+				  (invparity[par & 0x55] << 2) |
+				  (invparity[rp17] << 1) |
+				  (invparity[rp16] << 0);
+		break;
+	}
 
 	return 0;
 }
@@ -380,6 +411,7 @@ int ecc_sw_hamming_correct(unsigned char *buf, unsigned char *read_ecc,
 {
 	const u32 eccsize_mult = step_size >> 8;
 	unsigned char b0, b1, b2, bit_addr;
+	bool single_bit_err = false;
 	unsigned int byte_addr;
 
 	/*
@@ -388,6 +420,7 @@ int ecc_sw_hamming_correct(unsigned char *buf, unsigned char *read_ecc,
 	 * so keep them in a local var
 	*/
 	switch (ecc_order) {
+	case ECC_HAMMING_PL35X_ORDER:
 	case ECC_HAMMING_SM_ORDER:
 		b0 = read_ecc[0] ^ calc_ecc[0];
 		b1 = read_ecc[1] ^ calc_ecc[1];
@@ -407,33 +440,60 @@ int ecc_sw_hamming_correct(unsigned char *buf, unsigned char *read_ecc,
 	if ((b0 | b1 | b2) == 0)
 		return 0;	/* no error */
 
-	if ((((b0 ^ (b0 >> 1)) & 0x55) == 0x55) &&
-	    (((b1 ^ (b1 >> 1)) & 0x55) == 0x55) &&
-	    ((eccsize_mult == 1 && ((b2 ^ (b2 >> 1)) & 0x54) == 0x54) ||
-	     (eccsize_mult == 2 && ((b2 ^ (b2 >> 1)) & 0x55) == 0x55))) {
 	/* single bit error */
-		/*
-		 * rp17/rp15/13/11/9/7/5/3/1 indicate which byte is the faulty
-		 * byte, cp 5/3/1 indicate the faulty bit.
-		 * A lookup table (called addressbits) is used to filter
-		 * the bits from the byte they are in.
-		 * A marginal optimisation is possible by having three
-		 * different lookup tables.
-		 * One as we have now (for b0), one for b2
-		 * (that would avoid the >> 1), and one for b1 (with all values
-		 * << 4). However it was felt that introducing two more tables
-		 * hardly justify the gain.
-		 *
-		 * The b2 shift is there to get rid of the lowest two bits.
-		 * We could also do addressbits[b2] >> 1 but for the
-		 * performance it does not make any difference
-		 */
-		if (eccsize_mult == 1)
-			byte_addr = (addressbits[b1] << 4) + addressbits[b0];
-		else
-			byte_addr = (addressbits[b2 & 0x3] << 8) +
-				    (addressbits[b1] << 4) + addressbits[b0];
-		bit_addr = addressbits[b2 >> 2];
+	/*
+	 * rp17/rp15/13/11/9/7/5/3/1 indicate which byte is the faulty
+	 * byte, cp 5/3/1 indicate the faulty bit.
+	 */
+	switch (ecc_order) {
+	case ECC_HAMMING_SM_ORDER:
+	case ECC_HAMMING_REGULAR_ORDER:
+		if ((((b0 ^ (b0 >> 1)) & 0x55) == 0x55) &&
+		    (((b1 ^ (b1 >> 1)) & 0x55) == 0x55) &&
+		    ((eccsize_mult == 1 && ((b2 ^ (b2 >> 1)) & 0x54) == 0x54) ||
+		     (eccsize_mult == 2 && ((b2 ^ (b2 >> 1)) & 0x55) == 0x55))) {
+			single_bit_err = true;
+			/*
+			 * A lookup table (called addressbits) is used to filter
+			 * the bits from the byte they are in.
+			 * A marginal optimisation is possible by having three
+			 * different lookup tables.
+			 * One as we have now (for b0), one for b2
+			 * (that would avoid the >> 1), and one for b1 (with all values
+			 * << 4). However it was felt that introducing two more tables
+			 * hardly justify the gain.
+			 *
+			 * The b2 shift is there to get rid of the lowest two bits.
+			 * We could also do addressbits[b2] >> 1 but for the
+			 * performance it does not make any difference
+			 */
+			if (eccsize_mult == 1)
+				byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+			else
+				byte_addr = (addressbits[b2 & 0x3] << 8) +
+					    (addressbits[b1] << 4) + addressbits[b0];
+			bit_addr = addressbits[b2 >> 2];
+		}
+		break;
+	case ECC_HAMMING_PL35X_ORDER:
+		if (((((b0 >> 4) ^ b2) & 0xF) == 0xF) &&
+		    ((((b1 >> 4) ^ b0) & 0xF) == 0xF) &&
+		    ((((b2 >> 4) ^ b1) & 0xF) == 0xF)) {
+			single_bit_err = true;
+			/*
+			 * We can't use the addressbits table here because the
+			 * bit ordering in the ECC bytes has nothing to do with
+			 * the other ordering
+			 */
+			byte_addr = (b0 >> 3) & 0xF;
+			byte_addr |= (b0 >> 7) & 0x10;
+			byte_addr |= (b1 << 5) & 0x1e0;
+			bit_addr = b0 & 0x7;
+		}
+		break;
+	}
+
+	if (single_bit_err) {
 		/* flip the bit */
 		buf[byte_addr] ^= (1 << bit_addr);
 		return 1;
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index f6b6a4fa2b1f..c04c3e74f661 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5658,6 +5658,8 @@ int rawnand_sw_hamming_init(struct nand_chip *chip)
 
 	if (chip->ecc.options & NAND_ECC_SOFT_HAMMING_SM_ORDER)
 		engine_conf->ecc_order = ECC_HAMMING_SM_ORDER;
+	else if (chip->ecc.options & NAND_ECC_SOFT_HAMMING_PL35X_ORDER)
+		engine_conf->ecc_order = ECC_HAMMING_PL35X_ORDER;
 	else
 		engine_conf->ecc_order = ECC_HAMMING_REGULAR_ORDER;
 
diff --git a/include/linux/mtd/nand-ecc-sw-hamming.h b/include/linux/mtd/nand-ecc-sw-hamming.h
index ac6f28f35f77..4a0e08dc7f04 100644
--- a/include/linux/mtd/nand-ecc-sw-hamming.h
+++ b/include/linux/mtd/nand-ecc-sw-hamming.h
@@ -15,6 +15,7 @@
 enum ecc_hamming_order {
 	ECC_HAMMING_REGULAR_ORDER = 0,
 	ECC_HAMMING_SM_ORDER,
+	ECC_HAMMING_PL35X_ORDER,
 };
 
 /**
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 5c70e7bd3ed5..3658315752bf 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -113,10 +113,11 @@ struct gpio_desc;
  * When using software implementation of Hamming, we can specify which byte
  * ordering should be used.
  */
-#define NAND_ECC_SOFT_HAMMING_SM_ORDER	BIT(2)
+#define NAND_ECC_SOFT_HAMMING_SM_ORDER		BIT(2)
+#define NAND_ECC_SOFT_HAMMING_PL35X_ORDER	BIT(3)
 
 /* Chip has cache program function */
-#define NAND_CACHEPRG		BIT(3)
+#define NAND_CACHEPRG		BIT(4)
 /* Options valid for Samsung large page devices */
 #define NAND_SAMSUNG_LP_OPTIONS NAND_CACHEPRG
 

-- 
2.55.0


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

[PATCH 3/4] rawnand: base: Export nand_read_page_swecc

[Bastien Curutchet]

nand_read_page_swecc() isn't exported while it could be used from the
drivers for the mixed case where ECC writes are done by the hardware and
ECC reads are done by software.

Export nand_read_page_swecc() to make it usable by drivers.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c             | 5 +++--
 drivers/mtd/nand/raw/pl35x-nand-controller.c | 1 +
 include/linux/mtd/rawnand.h                  | 2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index c04c3e74f661..950c079aa27b 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -2941,8 +2941,8 @@ static int nand_read_page_raw_syndrome(struct nand_chip *chip, uint8_t *buf,
  * @oob_required: caller requires OOB data read to chip->oob_poi
  * @page: page number to read
  */
-static int nand_read_page_swecc(struct nand_chip *chip, uint8_t *buf,
-				int oob_required, int page)
+int nand_read_page_swecc(struct nand_chip *chip, uint8_t *buf,
+			 int oob_required, int page)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	int i, eccsize = chip->ecc.size, ret;
@@ -2979,6 +2979,7 @@ static int nand_read_page_swecc(struct nand_chip *chip, uint8_t *buf,
 	}
 	return max_bitflips;
 }
+EXPORT_SYMBOL_GPL(nand_read_page_swecc);
 
 /**
  * nand_read_subpage - [REPLACEABLE] ECC based sub-page read function
diff --git a/drivers/mtd/nand/raw/pl35x-nand-controller.c b/drivers/mtd/nand/raw/pl35x-nand-controller.c
index bd89aaadd1b2..d61eba938311 100644
--- a/drivers/mtd/nand/raw/pl35x-nand-controller.c
+++ b/drivers/mtd/nand/raw/pl35x-nand-controller.c
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/mtd/mtd.h>
+#include <linux/mtd/nand-ecc-sw-hamming.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/of.h>
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 3658315752bf..5539e96b2cac 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1578,6 +1578,8 @@ int nand_write_data_op(struct nand_chip *chip, const void *buf,
 		       unsigned int len, bool force_8bit);
 int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t *buf,
 				   int oob_required, int page);
+int nand_read_page_swecc(struct nand_chip *chip, uint8_t *buf,
+			 int oob_required, int page);
 
 /* Scan and identify a NAND device */
 int nand_scan_with_ids(struct nand_chip *chip, unsigned int max_chips,

-- 
2.55.0


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

[PATCH 4/4] rawnand: pl35x: Implement mixed ECC computing

[Bastien Curutchet]

The PL353's HW-ECC engine is broken and fails to report some errors
on reading (cf errata 721059 in [1]) so hardware reads can't be trusted.

Implement a mixed approach to do ECC writes by hardware and ECC reads by
software. It allows to benefit from the HW acceleration on writes and to
fix the read issue.
Use the hamming helpers provided by the core with the PL35x flag on to
do the soft computation.
Remove the no longer used read_hwecc().

[1] https://developer.arm.com/documentation/rlnc000227/a

Cc: stable@vger.kernel.org
Fixes: 08d8c62164a3 ("mtd: rawnand: pl353: Add support for the ARM PL353 SMC NAND controller")
Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/pl35x-nand-controller.c | 172 ++-------------------------
 1 file changed, 10 insertions(+), 162 deletions(-)

diff --git a/drivers/mtd/nand/raw/pl35x-nand-controller.c b/drivers/mtd/nand/raw/pl35x-nand-controller.c
index d61eba938311..78ddc5e8f311 100644
--- a/drivers/mtd/nand/raw/pl35x-nand-controller.c
+++ b/drivers/mtd/nand/raw/pl35x-nand-controller.c
@@ -392,50 +392,6 @@ static void pl35x_nand_write_data_op(struct nand_chip *chip, const u8 *out,
 		pl35x_smc_force_byte_access(chip, false);
 }
 
-static int pl35x_nand_correct_data(struct pl35x_nandc *nfc, unsigned char *buf,
-				   unsigned char *read_ecc,
-				   unsigned char *calc_ecc)
-{
-	unsigned short ecc_odd, ecc_even, read_ecc_lower, read_ecc_upper;
-	unsigned short calc_ecc_lower, calc_ecc_upper;
-	unsigned short byte_addr, bit_addr;
-
-	read_ecc_lower = (read_ecc[0] | (read_ecc[1] << 8)) &
-			 PL35X_NAND_ECC_BITS_MASK;
-	read_ecc_upper = ((read_ecc[1] >> 4) | (read_ecc[2] << 4)) &
-			 PL35X_NAND_ECC_BITS_MASK;
-
-	calc_ecc_lower = (calc_ecc[0] | (calc_ecc[1] << 8)) &
-			 PL35X_NAND_ECC_BITS_MASK;
-	calc_ecc_upper = ((calc_ecc[1] >> 4) | (calc_ecc[2] << 4)) &
-			 PL35X_NAND_ECC_BITS_MASK;
-
-	ecc_odd = read_ecc_lower ^ calc_ecc_lower;
-	ecc_even = read_ecc_upper ^ calc_ecc_upper;
-
-	/* No error */
-	if (likely(!ecc_odd && !ecc_even))
-		return 0;
-
-	/* One error in the main data; to be corrected */
-	if (ecc_odd == (~ecc_even & PL35X_NAND_ECC_BITS_MASK)) {
-		/* Bits [11:3] of error code give the byte offset */
-		byte_addr = (ecc_odd >> 3) & PL35X_NAND_ECC_BYTE_OFF_MASK;
-		/* Bits [2:0] of error code give the bit offset */
-		bit_addr = ecc_odd & PL35X_NAND_ECC_BIT_OFF_MASK;
-		/* Toggle the faulty bit */
-		buf[byte_addr] ^= (BIT(bit_addr));
-
-		return 1;
-	}
-
-	/* One error in the ECC data; no action needed */
-	if (hweight32(ecc_odd | ecc_even) == 1)
-		return 1;
-
-	return -EBADMSG;
-}
-
 static void pl35x_nand_ecc_reg_to_array(struct nand_chip *chip, u32 ecc_reg,
 					u8 *ecc_array)
 {
@@ -464,42 +420,6 @@ static int pl35x_nand_read_eccbytes(struct pl35x_nandc *nfc,
 	return 0;
 }
 
-static int pl35x_nand_recover_data_hwecc(struct pl35x_nandc *nfc,
-					 struct nand_chip *chip, u8 *data,
-					 u8 *read_ecc)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	unsigned int max_bitflips = 0, chunk;
-	u8 calc_ecc[3];
-	u32 ecc_value;
-	int stats;
-
-	for (chunk = 0; chunk < chip->ecc.steps;
-	     chunk++, data += chip->ecc.size, read_ecc += chip->ecc.bytes) {
-		/* Read ECC value for each chunk */
-		ecc_value = readl(nfc->conf_regs + PL35X_SMC_ECC_VALUE(chunk));
-
-		if (!PL35X_SMC_ECC_VALUE_IS_VALID(ecc_value))
-			return -EINVAL;
-
-		if (PL35X_SMC_ECC_VALUE_HAS_FAILED(ecc_value)) {
-			mtd->ecc_stats.failed++;
-			continue;
-		}
-
-		pl35x_nand_ecc_reg_to_array(chip, ecc_value, calc_ecc);
-		stats = pl35x_nand_correct_data(nfc, data, read_ecc, calc_ecc);
-		if (stats < 0) {
-			mtd->ecc_stats.failed++;
-		} else {
-			mtd->ecc_stats.corrected += stats;
-			max_bitflips = max_t(unsigned int, max_bitflips, stats);
-		}
-	}
-
-	return max_bitflips;
-}
-
 static int pl35x_nand_write_page_hwecc(struct nand_chip *chip,
 				       const u8 *buf, int oob_required,
 				       int page)
@@ -579,87 +499,6 @@ static int pl35x_nand_write_page_hwecc(struct nand_chip *chip,
 	return ret;
 }
 
-/*
- * This functions reads data and checks the data integrity by comparing hardware
- * generated ECC values and read ECC values from spare area.
- *
- * There is a limitation with SMC controller: ECC_LAST must be set on the
- * last data access to tell the ECC engine not to expect any further data.
- * In practice, this implies to shrink the last data transfert by eg. 4 bytes,
- * and doing a last 4-byte transfer with the additional bit set. The last block
- * should be aligned with the end of an ECC block. Because of this limitation,
- * it is not possible to use the core routines.
- */
-static int pl35x_nand_read_page_hwecc(struct nand_chip *chip,
-				      u8 *buf, int oob_required, int page)
-{
-	const struct nand_sdr_timings *sdr =
-		nand_get_sdr_timings(nand_get_interface_config(chip));
-	struct pl35x_nandc *nfc = to_pl35x_nandc(chip->controller);
-	struct pl35x_nand *plnand = to_pl35x_nand(chip);
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	unsigned int first_row = (mtd->writesize <= 512) ? 1 : 2;
-	unsigned int nrows = plnand->addr_cycles;
-	unsigned int addr1 = 0, addr2 = 0, row;
-	u32 cmd_addr;
-	int i, ret;
-
-	ret = pl35x_smc_set_ecc_mode(nfc, chip, PL35X_SMC_ECC_CFG_MODE_APB);
-	if (ret)
-		return ret;
-
-	cmd_addr = PL35X_SMC_CMD_PHASE |
-		   PL35X_SMC_CMD_PHASE_NADDRS(plnand->addr_cycles) |
-		   PL35X_SMC_CMD_PHASE_CMD0(NAND_CMD_READ0) |
-		   PL35X_SMC_CMD_PHASE_CMD1(NAND_CMD_READSTART) |
-		   PL35X_SMC_CMD_PHASE_CMD1_VALID;
-
-	for (i = 0, row = first_row; row < nrows; i++, row++) {
-		u8 addr = page >> ((i * 8) & 0xFF);
-
-		if (row < 4)
-			addr1 |= PL35X_SMC_CMD_PHASE_ADDR(row, addr);
-		else
-			addr2 |= PL35X_SMC_CMD_PHASE_ADDR(row - 4, addr);
-	}
-
-	/* Send the command and address cycles */
-	writel(addr1, nfc->io_regs + cmd_addr);
-	if (plnand->addr_cycles > 4)
-		writel(addr2, nfc->io_regs + cmd_addr);
-
-	/* Wait the data to be available in the NAND cache */
-	ndelay(PSEC_TO_NSEC(sdr->tRR_min));
-	ret = pl35x_smc_wait_for_irq(nfc);
-	if (ret)
-		goto disable_ecc_engine;
-
-	/* Retrieve the raw data with the engine enabled */
-	pl35x_nand_read_data_op(chip, buf, mtd->writesize, false,
-				0, PL35X_SMC_DATA_PHASE_ECC_LAST);
-	ret = pl35x_smc_wait_for_ecc_done(nfc);
-	if (ret)
-		goto disable_ecc_engine;
-
-	/* Retrieve the stored ECC bytes */
-	pl35x_nand_read_data_op(chip, chip->oob_poi, mtd->oobsize, false,
-				0, PL35X_SMC_DATA_PHASE_CLEAR_CS);
-	ret = mtd_ooblayout_get_eccbytes(mtd, nfc->ecc_buf, chip->oob_poi, 0,
-					 chip->ecc.total);
-	if (ret)
-		goto disable_ecc_engine;
-
-	pl35x_smc_set_ecc_mode(nfc, chip, PL35X_SMC_ECC_CFG_MODE_BYPASS);
-
-	/* Correct the data and report failures */
-	return pl35x_nand_recover_data_hwecc(nfc, chip, buf, nfc->ecc_buf);
-
-disable_ecc_engine:
-	pl35x_smc_set_ecc_mode(nfc, chip, PL35X_SMC_ECC_CFG_MODE_BYPASS);
-
-	return ret;
-}
-
 static int pl35x_nand_exec_op(struct nand_chip *chip,
 			      const struct nand_subop *subop)
 {
@@ -916,7 +755,7 @@ static int pl35x_nand_init_hw_ecc_controller(struct pl35x_nandc *nfc,
 	chip->ecc.bytes = 3;
 	chip->ecc.size = SZ_512;
 	chip->ecc.steps = mtd->writesize / chip->ecc.size;
-	chip->ecc.read_page = pl35x_nand_read_page_hwecc;
+	chip->ecc.read_page = nand_read_page_swecc;
 	chip->ecc.write_page = pl35x_nand_write_page_hwecc;
 	pl35x_smc_set_ecc_pg_size(nfc, chip, mtd->writesize);
 
@@ -939,6 +778,15 @@ static int pl35x_nand_init_hw_ecc_controller(struct pl35x_nandc *nfc,
 		return -EOPNOTSUPP;
 	}
 
+	chip->ecc.options |= NAND_ECC_SOFT_HAMMING_PL35X_ORDER;
+	ret = rawnand_sw_hamming_init(chip);
+	if (ret) {
+		WARN(1, "Hamming ECC initialization failed!\n");
+		return ret;
+	}
+	chip->ecc.calculate = rawnand_sw_hamming_calculate;
+	chip->ecc.correct = rawnand_sw_hamming_correct;
+
 	return ret;
 }
 

-- 
2.55.0


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