mtd: nand: mxs: Calculate ECC strength dynamically

PATCH Superseded Jörg Krause 2 participants 11 messages Apr 13, 2015 – Apr 15, 2015

Subsystems (3)

Arm i.MX and Layerscape Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Jörg Krause

author

2015-04-13 20:17 UTC #0 lore raw

v2 cover mtd: nand: mxs: Calculate ECC strength dynamically

This series of patches are based on the patch of Peng Fan:
https://patchwork.ozlabs.org/patch/422756/

Patch 1 is the originally patch from Peng Fan, Patch 2 and 3 add minor changes to 1 and patch 4 adds the ECC strength calculation to tools/mxsboot to be aligned with the changes made in patch 1 to 3.

Instead of hard-coding every possible oob size / ECC strength combination calculate the ECC strength dynamically to be aligned with the Linux Kernel MTD NAND driver. Also adds the calculation to tools/mxsboot to be aligned with the U-Boot MTD NAND driver.

Obviously, we have some code redundancy here in mxs_nand.c and mxsboot.c.

J?rg Krause (3):
mtd: nand: mxs: Replace magic number for bits per ECC level with macro
mtd: nand: mxs: Add comment for calculating ECC strength
tools: mxsboot: Calculate ECC strength dynamically

Peng Fan (1):
mtd:mxs:nand calculate ecc strength dynamically

 drivers/mtd/nand/mxs_nand.c | 36 +++++++++++++++---------------------
 tools/mxsboot.c             | 39 ++++++++++++++++++++++++---------------
 2 files changed, 39 insertions(+), 36 deletions(-)

Signature · 1 line

2.3.5

Jörg Krause

author

2015-04-13 20:17 UTC #1 lore raw

PATCH v2 1/4 mtd:mxs:nand calculate ecc strength dynamically

From: Peng Fan <[email protected]>

Calculate ecc strength according oobsize, but not hardcoded which is not aligned with kernel driver

Signed-off-by Peng Fan <[email protected]>

Signed-off-by Ye.Li <[email protected]>

Reviewed-by Marek Vasut <[email protected]>

Signed-off-by J?rg Krause <[email protected]>

---
 drivers/mtd/nand/mxs_nand.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

drivers/mtd/nand/mxs_nand.c +4 ~~-18~~

index 7a064ab..a45fcf9 100644
@@ -146,26 +146,12 @@ static uint32_t mxs_nand_aux_status_offset(void)
 static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
 						uint32_t page_oob_size)
 {
-	if (page_data_size == 2048) {
-		if (page_oob_size == 64)
-			return 8;
+	int ecc_strength;
 
-		if (page_oob_size == 112)
-			return 14;
-	}
-
-	if (page_data_size == 4096) {
-		if (page_oob_size == 128)
-			return 8;
-
-		if (page_oob_size == 218)
-			return 16;
+	ecc_strength = ((page_oob_size - MXS_NAND_METADATA_SIZE) * 8)
+			/ (13 * mxs_nand_ecc_chunk_cnt(page_data_size));
 
-		if (page_oob_size == 224)
-			return 16;
-	}
-
-	return 0;
+	return round_down(ecc_strength, 2);
 }
 
 static inline uint32_t mxs_nand_get_mark_offset(uint32_t page_data_size,

Signature · 1 line

2.3.5

Jörg Krause

author

2015-04-13 20:17 UTC #2 lore raw

PATCH v2 2/4 mtd: nand: mxs: Replace magic number for bits per ECC level with macro

In reply to Jörg Krause

Signed-off-by J?rg Krause <[email protected]>

---
 drivers/mtd/nand/mxs_nand.c | 7 ++++---
 tools/mxsboot.c             | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

drivers/mtd/nand/mxs_nand.c +4 -3

index a45fcf9..912fed8 100644
@@ -36,7 +36,7 @@
 #define	MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT	0
 #endif
 #define	MXS_NAND_METADATA_SIZE			10
-
+#define MXS_NAND_BITS_PER_ECC_LEVEL		13
 #define	MXS_NAND_COMMAND_BUFFER_SIZE		32
 
 #define	MXS_NAND_BCH_TIMEOUT			10000

@@ -135,7 +135,7 @@ static uint32_t mxs_nand_ecc_chunk_cnt(uint32_t page_data_size)
 
 static uint32_t mxs_nand_ecc_size_in_bits(uint32_t ecc_strength)
 {
-	return ecc_strength * 13;
+	return ecc_strength * MXS_NAND_BITS_PER_ECC_LEVEL;
 }
 
 static uint32_t mxs_nand_aux_status_offset(void)

@@ -149,7 +149,8 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
 	int ecc_strength;
 
 	ecc_strength = ((page_oob_size - MXS_NAND_METADATA_SIZE) * 8)
-			/ (13 * mxs_nand_ecc_chunk_cnt(page_data_size));
+			/ (MXS_NAND_BITS_PER_ECC_LEVEL *
+				mxs_nand_ecc_chunk_cnt(page_data_size));
 
 	return round_down(ecc_strength, 2);
 }

tools/mxsboot.c +2 -1

index 6d48cfb..aaa872b 100644
@@ -48,6 +48,7 @@ static uint32_t sd_sector = 2048;
 #define	MXS_NAND_DMA_DESCRIPTOR_COUNT		4
 #define	MXS_NAND_CHUNK_DATA_CHUNK_SIZE		512
 #define	MXS_NAND_METADATA_SIZE			10
+#define MXS_NAND_BITS_PER_ECC_LEVEL		13
 #define	MXS_NAND_COMMAND_BUFFER_SIZE		32
 
 struct mx28_nand_fcb {

@@ -127,7 +128,7 @@ struct mx28_sd_config_block {
 
 static inline uint32_t mx28_nand_ecc_size_in_bits(uint32_t ecc_strength)
 {
-	return ecc_strength * 13;
+	return ecc_strength * MXS_NAND_BITS_PER_ECC_LEVEL;
 }
 
 static inline uint32_t mx28_nand_get_ecc_strength(uint32_t page_data_size,

Signature · 1 line

2.3.5

Jörg Krause

author

2015-04-13 20:17 UTC #3 lore raw

PATCH v2 3/4 mtd: nand: mxs: Add comment for calculating ECC strength

In reply to Jörg Krause

Signed-off-by J?rg Krause <[email protected]>

---
 drivers/mtd/nand/mxs_nand.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/mtd/nand/mxs_nand.c +7 -0

index 912fed8..76e47ab 100644
@@ -148,6 +148,13 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
 {
 	int ecc_strength;
 
+	/* Determine the ECC layout with the formula:
+	 * 	ECC bits per chunk = (total page spare data bits) /
+	 * 		(bits per ECC level) / (chunks per page)
+	 * where:
+	 * 	total page spare data bits =
+	 * 		(page oob size - meta data size) * (bits per byte)
+	 */
 	ecc_strength = ((page_oob_size - MXS_NAND_METADATA_SIZE) * 8)
 			/ (MXS_NAND_BITS_PER_ECC_LEVEL *
 				mxs_nand_ecc_chunk_cnt(page_data_size));

Signature · 1 line

2.3.5

Heiko Schocher

2015-04-14 06:12 UTC #4 lore raw

PATCH v2 3/4 mtd: nand: mxs: Add comment for calculating ECC strength

Hello J?rg,

Am 13.04.2015 22:17, schrieb J?rg Krause:

Quoted 4 lines

Signed-off-by J?rg Krause <[email protected]>
---
  drivers/mtd/nand/mxs_nand.c | 7 +++++++
  1 file changed, 7 insertions(+)

nitpick only ...

drivers/mtd/nand/mxs_nand.c static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size, · line 151

Quoted 9 lines

drivers/mtd/nand/mxs_nand.c +1 -0

index 912fed8..76e47ab 100644
@@ -148,6 +148,13 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
  {
  	int ecc_strength;
 
+	/* Determine the ECC layout with the formula:

wrong comment style ... please fix also in patch 4/4... thanks.

bye,
Heiko

Quoted 10 lines

drivers/mtd/nand/mxs_nand.c +6 -0

+	 * 	ECC bits per chunk = (total page spare data bits) /
+	 * 		(bits per ECC level) / (chunks per page)
+	 * where:
+	 * 	total page spare data bits =
+	 * 		(page oob size - meta data size) * (bits per byte)
+	 */
  	ecc_strength = ((page_oob_size - MXS_NAND_METADATA_SIZE) * 8)
  			/ (MXS_NAND_BITS_PER_ECC_LEVEL *
  				mxs_nand_ecc_chunk_cnt(page_data_size));

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Jörg Krause

author

2015-04-14 06:29 UTC #5 lore raw

PATCH v2 3/4 mtd: nand: mxs: Add comment for calculating ECC strength

Hello Heiko,

Quoted 9 lines · Heiko Schocher

Hello J?rg,
Am 13.04.2015 22:17, schrieb J?rg Krause:
Quoted 4 lines
Signed-off-by J?rg Krause <[email protected]>
---
  drivers/mtd/nand/mxs_nand.c | 7 +++++++
  1 file changed, 7 insertions(+)
nitpick only ...

I'm unsure what this comment means translated to German. Something like small changes only?

Quoted 11 lines

drivers/mtd/nand/mxs_nand.c +0 -0

index 912fed8..76e47ab 100644
@@ -148,6 +148,13 @@ static inline uint32_t 
 mxs_nand_get_ecc_strength(uint32_t page_data_size,
  {
    int ecc_strength;

Quoted 3 lines

drivers/mtd/nand/mxs_nand.c static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size, · line 151
Quoted 1 line
drivers/mtd/nand/mxs_nand.c +1 -0
+   /* Determine the ECC layout with the formula:
wrong comment style ... please fix also in patch 4/4... thanks.

Checkpatch did not complain and I did not know there is a coding style for comments. Should it be:
/*
* Determine the ECC layout...

Bye
J?rg

Heiko Schocher

2015-04-14 08:02 UTC #6 lore raw

PATCH v2 3/4 mtd: nand: mxs: Add comment for calculating ECC strength

Hello J?rg,

Am 14.04.2015 08:29, schrieb J?rg Krause:

Quoted 11 lines

Hello Heiko,
Quoted 8 lines · Heiko Schocher
Hello J?rg,
Am 13.04.2015 22:17, schrieb J?rg Krause:
Quoted 4 lines
Signed-off-by J?rg Krause <[email protected]>
---
   drivers/mtd/nand/mxs_nand.c | 7 +++++++
   1 file changed, 7 insertions(+)

Quoted 4 lines

Quoted 1 line
nitpick only ...
I'm unsure what this comment means translated to German. Something like small changes only?

Yes, something like "pingelig"

Quoted 10 lines

drivers/mtd/nand/mxs_nand.c +0 -0

index 912fed8..76e47ab 100644
@@ -148,6 +148,13 @@ static inline uint32_t
 mxs_nand_get_ecc_strength(uint32_t page_data_size,
   {
     int ecc_strength;

Quoted 8 lines

Quoted 3 lines
drivers/mtd/nand/mxs_nand.c static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size, · line 151
Quoted 1 line
drivers/mtd/nand/mxs_nand.c +1 -0
+   /* Determine the ECC layout with the formula:
wrong comment style ... please fix also in patch 4/4... thanks.
Checkpatch did not complain and I did not know there is a coding style for comments. Should it be:
/*
* Determine the ECC layout...

Yes.

See: linux:/Documentation/CodingStyle search for "The preferred style for long (multi-line) comments is"

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Jörg Krause

author

2015-04-14 15:32 UTC #7 lore raw

PATCH v2 3/4 mtd: nand: mxs: Add comment for calculating ECC strength

Hello Heiko,

Quoted 14 lines · Heiko Schocher

Hello J?rg,
Am 14.04.2015 08:29, schrieb J?rg Krause:
Quoted 11 lines
Hello Heiko,
Quoted 8 lines · Heiko Schocher
Hello J?rg,
Am 13.04.2015 22:17, schrieb J?rg Krause:
Quoted 4 lines
Signed-off-by J?rg Krause <[email protected]>
---
   drivers/mtd/nand/mxs_nand.c | 7 +++++++
   1 file changed, 7 insertions(+)

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
nitpick only ...
I'm unsure what this comment means translated to German. Something like small changes only?
Yes, something like "pingelig"

Okay, now I understand the translation but not the meaning. Do you mean that this patch should be squashed into patch 1 for example?

Quoted 21 lines

Quoted 15 lines
Quoted 11 lines
Quoted 11 lines
drivers/mtd/nand/mxs_nand.c +0 -0
index 912fed8..76e47ab 100644
@@ -148,6 +148,13 @@ static inline uint32_t
 mxs_nand_get_ecc_strength(uint32_t page_data_size,
   {
     int ecc_strength;
 
Quoted 3 lines
drivers/mtd/nand/mxs_nand.c static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size, · line 151
Quoted 1 line
drivers/mtd/nand/mxs_nand.c +1 -0
+   /* Determine the ECC layout with the formula:
wrong comment style ... please fix also in patch 4/4... thanks.
Quoted 5 lines
Checkpatch did not complain and I did not know there is a coding style
for comments. Should it be:
/*
* Determine the ECC layout...

Quoted 4 lines

Yes.
See: linux:/Documentation/CodingStyle search for "The preferred style for long (multi-line) comments is"

Thanks! So shall I send a v3 for this patch or will this be fixed by a maintainer?

Best regards
J?rg Krause

Heiko Schocher

2015-04-15 05:39 UTC #8 lore raw

PATCH v2 3/4 mtd: nand: mxs: Add comment for calculating ECC strength

Hello J?rg,

Am 14.04.2015 17:32, schrieb J?rg Krause:

Quoted 48 lines

Hello Heiko,
Quoted 14 lines · Heiko Schocher
Hello J?rg,
Am 14.04.2015 08:29, schrieb J?rg Krause:
Quoted 11 lines
Hello Heiko,
Quoted 8 lines · Heiko Schocher
Hello J?rg,
Am 13.04.2015 22:17, schrieb J?rg Krause:
Quoted 4 lines
Signed-off-by J?rg Krause <[email protected]>
---
    drivers/mtd/nand/mxs_nand.c | 7 +++++++
    1 file changed, 7 insertions(+)
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
nitpick only ...
I'm unsure what this comment means translated to German. Something like small changes only?
Yes, something like "pingelig"
Okay, now I understand the translation but not the meaning. Do you mean that this patch should be squashed into patch 1 for example?
Quoted 21 lines
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
drivers/mtd/nand/mxs_nand.c +0 -0
index 912fed8..76e47ab 100644
@@ -148,6 +148,13 @@ static inline uint32_t
 mxs_nand_get_ecc_strength(uint32_t page_data_size,
    {
      int ecc_strength;
 
Quoted 3 lines
drivers/mtd/nand/mxs_nand.c static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size, · line 151
Quoted 1 line
drivers/mtd/nand/mxs_nand.c +1 -0
+   /* Determine the ECC layout with the formula:
wrong comment style ... please fix also in patch 4/4... thanks.
Checkpatch did not complain and I did not know there is a coding style
for comments. Should it be:
/*
* Determine the ECC layout...

Quoted 7 lines

Quoted 4 lines
Yes.
See: linux:/Documentation/CodingStyle search for "The preferred style for long (multi-line) comments is"
Thanks! So shall I send a v3 for this patch or will this be fixed by a maintainer?

Please sent a v3 ... and add Scott Wood <[email protected]> to Cc as he is the nand maintainer, thanks!

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Jörg Krause

author

2015-04-13 20:17 UTC #9 lore raw

PATCH v2 4/4 tools: mxsboot: Calculate ECC strength dynamically

In reply to Jörg Krause

Calculating the ECC strength dynamically to be aligned with the mxs NAND driver and the Linux Kernel.

The macro definition for round_down is taken from <linux/kernel.h> to avoid changing the tools/Makefile where the Linux Kernel header files are not included for building the tools target.

Signed-off-by J?rg Krause <[email protected]>

---
 tools/mxsboot.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

tools/mxsboot.c +22 ~~-14~~

index aaa872b..8fca8cf 100644
@@ -14,6 +14,10 @@
 
 #include "compiler.h"
 
+/* Taken from <linux/kernel.h> */
+#define __round_mask(x, y) ((__typeof__(x))((y)-1))
+#define round_down(x, y) ((x) & ~__round_mask(x, y))
+
 /*
  * Default BCB layout.
  *

@@ -126,6 +130,11 @@ struct mx28_sd_config_block {
 	struct mx28_sd_drive_info	drv_info[1];
 };
 
+static inline uint32_t mx28_nand_ecc_chunk_cnt(uint32_t page_data_size)
+{
+	return page_data_size / MXS_NAND_CHUNK_DATA_CHUNK_SIZE;
+}
+
 static inline uint32_t mx28_nand_ecc_size_in_bits(uint32_t ecc_strength)
 {
 	return ecc_strength * MXS_NAND_BITS_PER_ECC_LEVEL;

@@ -134,21 +143,20 @@ static inline uint32_t mx28_nand_ecc_size_in_bits(uint32_t ecc_strength)
 static inline uint32_t mx28_nand_get_ecc_strength(uint32_t page_data_size,
 						uint32_t page_oob_size)
 {
-	if (page_data_size == 2048)
-		return 8;
-
-	if (page_data_size == 4096) {
-		if (page_oob_size == 128)
-			return 8;
-
-		if (page_oob_size == 218)
-			return 16;
-
-		if (page_oob_size == 224)
-			return 16;
-	}
+	int ecc_strength;
+
+	/* Determine the ECC layout with the formula:
+	 * 	ECC bits per chunk = (total page spare data bits) /
+	 * 		(bits per ECC level) / (chunks per page)
+	 * where:
+	 * 	total page spare data bits =
+	 * 		(page oob size - meta data size) * (bits per byte)
+	 */
+	ecc_strength = ((page_oob_size - MXS_NAND_METADATA_SIZE) * 8)
+			/ (MXS_NAND_BITS_PER_ECC_LEVEL *
+				mx28_nand_ecc_chunk_cnt(page_data_size));
 
-	return 0;
+	return round_down(ecc_strength, 2);
 }
 
 static inline uint32_t mx28_nand_get_mark_offset(uint32_t page_data_size,

Signature · 1 line

2.3.5

Heiko Schocher

2015-04-14 06:11 UTC #10 lore raw

v2 cover mtd: nand: mxs: Calculate ECC strength dynamically

In reply to Jörg Krause

Hello J?rg,

Am 13.04.2015 22:17, schrieb J?rg Krause:

Quoted 19 lines

This series of patches are based on the patch of Peng Fan:
https://patchwork.ozlabs.org/patch/422756/
Patch 1 is the originally patch from Peng Fan, Patch 2 and 3 add minor changes to 1 and patch 4 adds the ECC strength calculation to tools/mxsboot to be aligned with the changes made in patch 1 to 3.
Instead of hard-coding every possible oob size / ECC strength combination calculate the ECC strength dynamically to be aligned with the Linux Kernel MTD NAND driver. Also adds the calculation to tools/mxsboot to be aligned with the U-Boot MTD NAND driver.
Obviously, we have some code redundancy here in mxs_nand.c and mxsboot.c.
J?rg Krause (3):
mtd: nand: mxs: Replace magic number for bits per ECC level with macro
mtd: nand: mxs: Add comment for calculating ECC strength
tools: mxsboot: Calculate ECC strength dynamically

Quoted 6 lines

Peng Fan (1):
mtd:mxs:nand calculate ecc strength dynamically

  drivers/mtd/nand/mxs_nand.c | 36 +++++++++++++++---------------------
  tools/mxsboot.c             | 39 ++++++++++++++++++++++++---------------
  2 files changed, 39 insertions(+), 36 deletions(-)

Tried this patches on the aristainetos2 board. UBI on the nand works nice with this patches, so:

Acked-by Heiko Schocher <[email protected]>

This patchset replaces my patch:

Patchwork [U-Boot] mxs_nand: Fix ECC strength for NAND flash with OOB size of 256
http://patchwork.ozlabs.org/patch/460462/

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany