[U-Boot] [PATCH v2 0/4] mtd: nand: mxs: Calculate ECC strength dynamically

[U-Boot] [PATCH v2 0/4] mtd: nand: mxs: Calculate ECC strength dynamically

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

-- 
2.3.5

[U-Boot] [PATCH v2 1/4] mtd:mxs:nand calculate ecc strength dynamically

[Jörg Krause]

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

[U-Boot] [PATCH v2 2/4] mtd: nand: mxs: Replace magic number for bits per ECC level with macro

[Jörg Krause]

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

[U-Boot] [PATCH v2 3/4] mtd: nand: mxs: Add comment for calculating ECC strength

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

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

[U-Boot] [PATCH v2 4/4] tools: mxsboot: Calculate ECC strength dynamically

[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 <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,
-- 
2.3.5

[U-Boot] [PATCH v2 0/4] mtd: nand: mxs: Calculate ECC strength dynamically

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH v2 3/4] mtd: nand: mxs: Add comment for calculating ECC strength

[Heiko Schocher]

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)
> +	 */
>   	ecc_strength = ((page_oob_size - MXS_NAND_METADATA_SIZE) * 8)
>   			/ (MXS_NAND_BITS_PER_ECC_LEVEL *
>   				mxs_nand_ecc_chunk_cnt(page_data_size));
>

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

[U-Boot] [PATCH v2 3/4] mtd: nand: mxs: Add comment for calculating ECC strength

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

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

[U-Boot] [PATCH v2 3/4] mtd: nand: mxs: Add comment for calculating ECC strength

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH v2 3/4] mtd: nand: mxs: Add comment for calculating ECC strength

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

Best regards
J?rg Krause

[U-Boot] [PATCH v2 3/4] mtd: nand: mxs: Add comment for calculating ECC strength

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany