[U-Boot] [PATCH v2 0/4] 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(-)
From: Peng Fan Peng.Fan@freescale.com
Calculate ecc strength according oobsize, but not hardcoded which is not aligned with kernel driver
Signed-off-by: Peng Fan Peng.Fan@freescale.com Signed-off-by: Ye.Li b37916@freescale.com Reviewed-by: Marek Vasut marex@denx.de Signed-off-by: Jörg Krause joerg.krause@embedded.rocks --- drivers/mtd/nand/mxs_nand.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index 7a064ab..a45fcf9 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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,
Signed-off-by: Jörg Krause joerg.krause@embedded.rocks --- drivers/mtd/nand/mxs_nand.c | 7 ++++--- tools/mxsboot.c | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index a45fcf9..912fed8 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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); } diff --git a/tools/mxsboot.c b/tools/mxsboot.c index 6d48cfb..aaa872b 100644 --- a/tools/mxsboot.c +++ b/tools/mxsboot.c @@ -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,
Signed-off-by: Jörg Krause joerg.krause@embedded.rocks --- drivers/mtd/nand/mxs_nand.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index 912fed8..76e47ab 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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));
Hello Jörg,
Am 13.04.2015 22:17, schrieb Jörg Krause:
Signed-off-by: Jörg Krause joerg.krause@embedded.rocks
drivers/mtd/nand/mxs_nand.c | 7 +++++++ 1 file changed, 7 insertions(+)
nitpick only ...
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index 912fed8..76e47ab 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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
* 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)*/
Hello Heiko,
On Di, 2015-04-14 at 08:12 +0200, Heiko Schocher wrote:
Hello Jörg,
Am 13.04.2015 22:17, schrieb Jörg Krause:
Signed-off-by: Jörg Krause joerg.krause@embedded.rocks
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?
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index 912fed8..76e47ab 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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.
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
Hello Jörg,
Am 14.04.2015 08:29, schrieb Jörg Krause:
Hello Heiko,
On Di, 2015-04-14 at 08:12 +0200, Heiko Schocher wrote:
Hello Jörg,
Am 13.04.2015 22:17, schrieb Jörg Krause:
Signed-off-by: Jörg Krause joerg.krause@embedded.rocks
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?
Yes, something like "pingelig"
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index 912fed8..76e47ab 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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.
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
Hello Heiko,
On Di, 2015-04-14 at 10:02 +0200, Heiko Schocher wrote:
Hello Jörg,
Am 14.04.2015 08:29, schrieb Jörg Krause:
Hello Heiko,
On Di, 2015-04-14 at 08:12 +0200, Heiko Schocher wrote:
Hello Jörg,
Am 13.04.2015 22:17, schrieb Jörg Krause:
Signed-off-by: Jörg Krause joerg.krause@embedded.rocks
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?
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?
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index 912fed8..76e47ab 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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.
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"
Thanks! So shall I send a v3 for this patch or will this be fixed by a maintainer?
Best regards Jörg Krause
Hello Jörg,
Am 14.04.2015 17:32, schrieb Jörg Krause:
Hello Heiko,
On Di, 2015-04-14 at 10:02 +0200, Heiko Schocher wrote:
Hello Jörg,
Am 14.04.2015 08:29, schrieb Jörg Krause:
Hello Heiko,
On Di, 2015-04-14 at 08:12 +0200, Heiko Schocher wrote:
Hello Jörg,
Am 13.04.2015 22:17, schrieb Jörg Krause:
Signed-off-by: Jörg Krause joerg.krause@embedded.rocks
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?
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?
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index 912fed8..76e47ab 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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.
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"
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 scottwood@freescale.com to Cc as he is the nand maintainer, thanks!
bye, Heiko
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 joerg.krause@embedded.rocks --- tools/mxsboot.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/tools/mxsboot.c b/tools/mxsboot.c index aaa872b..8fca8cf 100644 --- a/tools/mxsboot.c +++ b/tools/mxsboot.c @@ -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,
Hello Jörg,
Am 13.04.2015 22:17, schrieb Jörg Krause:
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(-)
Tried this patches on the aristainetos2 board. UBI on the nand works nice with this patches, so:
Acked-by: Heiko Schocher hs@denx.de
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
participants (2)
-
Heiko Schocher -
Jörg Krause