Skip to contentSkip to conversation

mtd: nand: mxs support oobsize bigger than 512

PATCH RESEND New Peng Fan 6 participants 24 messages Jul 21, 2015 – Aug 26, 2015

Subsystems (4)
Arm (32-bit) Arm i.MX and Layerscape Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

Peng Fan

author
#0 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

If ecc chunk data size is 512 and oobsize is bigger than 512, there is a chance that block_mark_bit_offset conflicts with bch ecc area.

The following graph is modified from kernel gpmi-nand.c driver with each data block 512 bytes. We can see that Block Mark conflicts with ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

   |                          P                                        |
   |<----------------------------------------------------------------->|
   |                                                                   |
   |                                                (Block Mark)       |
   |                      P'                             |           | |   |
   |<--------------------------------------------------->|     D     | | O'|
   |                                                     |<--------->| |<->|
   V                                                     V           V V   V
   +---+--------------+-+--------------+-+--------------+-+----------+-+---+
   | M |   data       |E|   data       |E|   data       |E|   data   |E|   |
   +---+--------------+-+--------------+-+--------------+-+----------+-+---+
                                                        ^                  ^
                                                        |         O        |
                                                        |<---------------->|

P : the page size for BCH module.
E : The ECC strength.
G : the length of Galois Field.
N : The chunk count of per page.
M : the metasize of per page.
C : the ecc chunk size, aka the "data" above. P': the nand chip's page size.
O : the nand chip's oob size.
O': the free oob.

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

---

Changes v2:
none

Changes v1:
The previous patch is https://patchwork.ozlabs.org/patch/422757/
This version contains a minor change to the name from gf_len to galois_field. Also add Marek's Reviewed-by.

 arch/arm/include/asm/imx-common/regs-bch.h |  2 ++
 drivers/mtd/nand/mxs_nand.c                | 32 +++++++++++++++++++++---------
 2 files changed, 25 insertions(+), 9 deletions(-)
arch/arm/include/asm/imx-common/regs-bch.h +2 -0
index a33d341..5c47783 100644
@@ -148,6 +148,7 @@ struct mxs_bch_regs {
 #define	BCH_FLASHLAYOUT0_ECC0_ECC30			(0xf << 12)
 #define	BCH_FLASHLAYOUT0_ECC0_ECC32			(0x10 << 12)
 #define	BCH_FLASHLAYOUT0_GF13_0_GF14_1			(1 << 10)
+#define	BCH_FLASHLAYOUT0_GF13_0_GF14_1_OFFSET		10
 #define	BCH_FLASHLAYOUT0_DATA0_SIZE_MASK		0xfff
 #define	BCH_FLASHLAYOUT0_DATA0_SIZE_OFFSET		0
 

@@ -178,6 +179,7 @@ struct mxs_bch_regs {
 #define	BCH_FLASHLAYOUT1_ECCN_ECC30			(0xf << 12)
 #define	BCH_FLASHLAYOUT1_ECCN_ECC32			(0x10 << 12)
 #define	BCH_FLASHLAYOUT1_GF13_0_GF14_1			(1 << 10)
+#define	BCH_FLASHLAYOUT1_GF13_0_GF14_1_OFFSET		10
 #define	BCH_FLASHLAYOUT1_DATAN_SIZE_MASK		0xfff
 #define	BCH_FLASHLAYOUT1_DATAN_SIZE_OFFSET		0
drivers/mtd/nand/mxs_nand.c +23 -9
index 9c144a2..33ce817 100644
@@ -68,6 +68,8 @@ struct mxs_nand_info {
 };
 
 struct nand_ecclayout fake_ecc_layout;
+static int chunk_data_size = MXS_NAND_CHUNK_DATA_CHUNK_SIZE;
+static int galois_field = 13;
 
 /*
  * Cache management functions

@@ -130,12 +132,12 @@ static void mxs_nand_return_dma_descs(struct mxs_nand_info *info)
 
 static uint32_t mxs_nand_ecc_chunk_cnt(uint32_t page_data_size)
 {
-	return page_data_size / MXS_NAND_CHUNK_DATA_CHUNK_SIZE;
+	return page_data_size / chunk_data_size;
 }
 
 static uint32_t mxs_nand_ecc_size_in_bits(uint32_t ecc_strength)
 {
-	return ecc_strength * MXS_NAND_BITS_PER_ECC_LEVEL;
+	return ecc_strength * galois_field;
 }
 
 static uint32_t mxs_nand_aux_status_offset(void)

@@ -157,8 +159,8 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
 	 *		(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));
+			/ (galois_field *
+			   mxs_nand_ecc_chunk_cnt(page_data_size));
 
 	return round_down(ecc_strength, 2);
 }

@@ -173,7 +175,7 @@ static inline uint32_t mxs_nand_get_mark_offset(uint32_t page_data_size,
 	uint32_t block_mark_chunk_bit_offset;
 	uint32_t block_mark_bit_offset;
 
-	chunk_data_size_in_bits = MXS_NAND_CHUNK_DATA_CHUNK_SIZE * 8;
+	chunk_data_size_in_bits = chunk_data_size * 8;
 	chunk_ecc_size_in_bits  = mxs_nand_ecc_size_in_bits(ecc_strength);
 
 	chunk_total_size_in_bits =

@@ -972,6 +974,16 @@ static int mxs_nand_scan_bbt(struct mtd_info *mtd)
 	struct mxs_bch_regs *bch_regs = (struct mxs_bch_regs *)MXS_BCH_BASE;
 	uint32_t tmp;
 
+	if (mtd->oobsize > MXS_NAND_CHUNK_DATA_CHUNK_SIZE) {
+		galois_field = 14;
+		chunk_data_size = MXS_NAND_CHUNK_DATA_CHUNK_SIZE * 2;
+	}
+
+	if (mtd->oobsize > chunk_data_size) {
+		printf("Not support the NAND chips whose oob size is larger then %d bytes!\n", chunk_data_size);
+		return -EINVAL;
+	}
+
 	/* Configure BCH and set NFC geometry */
 	mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
 

@@ -981,16 +993,18 @@ static int mxs_nand_scan_bbt(struct mtd_info *mtd)
 	tmp |= MXS_NAND_METADATA_SIZE << BCH_FLASHLAYOUT0_META_SIZE_OFFSET;
 	tmp |= (mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize) >> 1)
 		<< BCH_FLASHLAYOUT0_ECC0_OFFSET;
-	tmp |= MXS_NAND_CHUNK_DATA_CHUNK_SIZE
-		>> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
+	tmp |= chunk_data_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
+	tmp |= (14 == galois_field ? 1 : 0) <<
+		BCH_FLASHLAYOUT0_GF13_0_GF14_1_OFFSET;
 	writel(tmp, &bch_regs->hw_bch_flash0layout0);
 
 	tmp = (mtd->writesize + mtd->oobsize)
 		<< BCH_FLASHLAYOUT1_PAGE_SIZE_OFFSET;
 	tmp |= (mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize) >> 1)
 		<< BCH_FLASHLAYOUT1_ECCN_OFFSET;
-	tmp |= MXS_NAND_CHUNK_DATA_CHUNK_SIZE
-		>> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
+	tmp |= chunk_data_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
+	tmp |= (14 == galois_field ? 1 : 0) <<
+		BCH_FLASHLAYOUT1_GF13_0_GF14_1_OFFSET;
 	writel(tmp, &bch_regs->hw_bch_flash0layout1);
 
 	/* Set *all* chip selects to use layout 0 */
Signature · 1 line
1.8.4
author
#1 lore raw

PATCH v2 2/3 mtd: nand: mxs check maximum ecc that platfrom supports

Check maximum ecc strength for each platfrom to avoid the calculated ecc exceed the limitation.

Signed-off-by Peng Fan <[email protected]>
Signed-off-by Han Xu <[email protected]>
Reviewed-by Marek Vasut <[email protected]>

---

Changes v2:
Add Marek's reviewed by.

 drivers/mtd/nand/mxs_nand.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +8 -1
index 33ce817..97011b2 100644
@@ -149,6 +149,13 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
 						uint32_t page_oob_size)
 {
 	int ecc_strength;
+	int max_ecc_strength_supported;
+
+	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+	if (is_cpu_type(MXC_CPU_MX6SX))
+		max_ecc_strength_supported = 62;
+	else
+		max_ecc_strength_supported = 40;
 
 	/*
 	 * Determine the ECC layout with the formula:

@@ -162,7 +169,7 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
 			/ (galois_field *
 			   mxs_nand_ecc_chunk_cnt(page_data_size));
 
-	return round_down(ecc_strength, 2);
+	return min(round_down(ecc_strength, 2), max_ecc_strength_supported);
 }
 
 static inline uint32_t mxs_nand_get_mark_offset(uint32_t page_data_size,
Signature · 1 line
1.8.4
#2 lore raw

PATCH v2 2/3 mtd: nand: mxs check maximum ecc that platfrom supports

Quoted 42 lines · Peng Fan

Check maximum ecc strength for each platfrom to avoid the calculated ecc exceed the limitation.

Signed-off-by Peng Fan <[email protected]>
Signed-off-by Han Xu <[email protected]>
Reviewed-by Marek Vasut <[email protected]>

---

Changes v2:
Add Marek's reviewed by.

 drivers/mtd/nand/mxs_nand.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +8 -2
index 33ce817..97011b2 100644
@@ -149,6 +149,13 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
                                                uint32_t page_oob_size)
 {
        int ecc_strength;
+       int max_ecc_strength_supported;
+
+       /* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+       if (is_cpu_type(MXC_CPU_MX6SX))
+               max_ecc_strength_supported = 62;
+       else
+               max_ecc_strength_supported = 40;
 
        /*
         * Determine the ECC layout with the formula:

@@ -162,7 +169,7 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
                        / (galois_field *
                           mxs_nand_ecc_chunk_cnt(page_data_size));
 
-       return round_down(ecc_strength, 2);
+       return min(round_down(ecc_strength, 2), max_ecc_strength_supported);
 }
 
 static inline uint32_t mxs_nand_get_mark_offset(uint32_t page_data_size,
--
Quoted 2 lines

1.8.4

Tested on IMX6 with MT29F16G08 16Gb and MT29F2G08 2Gb NAND devices.

Tested-by Tim Harvey <[email protected]>
#3 lore raw

PATCH v2 2/3 mtd: nand: mxs check maximum ecc that platfrom supports

In reply to Peng Fan
Quoted 20 lines · Peng Fan

Check maximum ecc strength for each platfrom to avoid the calculated ecc exceed the limitation.

Signed-off-by Peng Fan <[email protected]>
Signed-off-by Han Xu <[email protected]>
Reviewed-by Marek Vasut <[email protected]>

---

Changes v2:
Add Marek's reviewed by.

 drivers/mtd/nand/mxs_nand.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +0 -0
index 33ce817..97011b2 100644
@@ -149,6 +149,13 @@ static inline uint32_t 
 mxs_nand_get_ecc_strength(uint32_t page_data_size,

drivers/mtd/nand/mxs_nand.c static inline uint32_t · line 159

Quoted 10 lines
drivers/mtd/nand/mxs_nand.c +7 -0
                                              uint32_t page_oob_size)
 {
      int ecc_strength;
+     int max_ecc_strength_supported;
+
+     /* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+     if (is_cpu_type(MXC_CPU_MX6SX))
+             max_ecc_strength_supported = 62;
+     else
+             max_ecc_strength_supported = 40;

arm: + mx28evk_nand
+drivers/mtd/nand/mxs_nand.c:155:18: error: 'MXC_CPU_MX6SX' undeclared (first use in this function) +drivers/mtd/nand/mxs_nand.c:155:18: note: each undeclared identifier is reported only once for each function it appears in +make[2]: *** [drivers/mtd/nand/mxs_nand.o] Error 1
+make[1]: *** [drivers/mtd/nand] Error 2
+make: *** [sub-make] Error 2

(among other failed targets)

I tried to fix it by including asm/arch-imx/cpu.h, but then got undefined reference to is_cpu_type().

-Scott

#4 lore raw

PATCH v2 2/3 mtd: nand: mxs check maximum ecc that platfrom supports

Hi Scott,

Quoted 33 lines · Scott Wood
Quoted 20 lines · Peng Fan

Check maximum ecc strength for each platfrom to avoid the calculated ecc exceed the limitation.

Signed-off-by Peng Fan <[email protected]>
Signed-off-by Han Xu <[email protected]>
Reviewed-by Marek Vasut <[email protected]>

---

Changes v2:
Add Marek's reviewed by.

 drivers/mtd/nand/mxs_nand.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +0 -0
index 33ce817..97011b2 100644
@@ -149,6 +149,13 @@ static inline uint32_t 
 mxs_nand_get_ecc_strength(uint32_t page_data_size,

drivers/mtd/nand/mxs_nand.c static inline uint32_t · line 159

Quoted 10 lines
drivers/mtd/nand/mxs_nand.c +7 -0
                                              uint32_t page_oob_size)
 {
      int ecc_strength;
+     int max_ecc_strength_supported;
+
+     /* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+     if (is_cpu_type(MXC_CPU_MX6SX))
+             max_ecc_strength_supported = 62;
+     else
+             max_ecc_strength_supported = 40;

arm: + mx28evk_nand

Review

Quoted 5 lines
(quoted diff) +5 -0
+drivers/mtd/nand/mxs_nand.c:155:18: error: 'MXC_CPU_MX6SX' undeclared (first use in this function)
+drivers/mtd/nand/mxs_nand.c:155:18: note: each undeclared identifier is reported only once for each function it appears in
+make[2]: *** [drivers/mtd/nand/mxs_nand.o] Error 1
+make[1]: *** [drivers/mtd/nand] Error 2
+make: *** [sub-make] Error 2

My bad. I only take mx6 into consideration when I did this patch.

Quoted 5 lines

(among other failed targets)

I tried to fix it by including asm/arch-imx/cpu.h, but then got undefined reference to is_cpu_type().

Now is_cpu_type() is common to all i.MXes, but need this patch http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commitdiff;h=bf3303c98b23a29d99acc5e28865db604873b699 which is still in imx tree now. After applying the following patches, is_cpu_type should work.
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=7c015efa7f28911d538ddd7d12926edd95176791
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=8b647df9cae0ca65656cca19d4bd43239a9eca42
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=3e26a2224a828cd07d1d1df46cbea7e99cc87993
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=11caa3fa9dae79864726fcb763f100b4fd713884
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=bf3303c98b23a29d99acc5e28865db604873b699

Then do you need me to repost this patch after the upper five patches in imx tree merge into uboot master branch, or the 3 patches in this patchset go through imx tree?
https://patchwork.ozlabs.org/patch/498048/
https://patchwork.ozlabs.org/patch/498049/
https://patchwork.ozlabs.org/patch/498050/

Thanks,
Peng.

Quoted 3 lines

-Scott

#5 lore raw

PATCH v2 2/3 mtd: nand: mxs check maximum ecc that platfrom supports

Quoted 62 lines · Peng Fan

Hi Scott,

Quoted 37 lines · Scott Wood
Quoted 20 lines · Peng Fan

Check maximum ecc strength for each platfrom to avoid the calculated ecc exceed the limitation.

Signed-off-by Peng Fan <[email protected]>
Signed-off-by Han Xu <[email protected]>
Reviewed-by Marek Vasut <[email protected]>

---

Changes v2:
Add Marek's reviewed by.

 drivers/mtd/nand/mxs_nand.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +0 -0
index 33ce817..97011b2 100644
@@ -149,6 +149,13 @@ static inline uint32_t 
 mxs_nand_get_ecc_strength(uint32_t page_data_size,

drivers/mtd/nand/mxs_nand.c static inline uint32_t · line 159

Quoted 10 lines
drivers/mtd/nand/mxs_nand.c +7 -0
                                              uint32_t page_oob_size)
 {
      int ecc_strength;
+     int max_ecc_strength_supported;
+
+     /* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+     if (is_cpu_type(MXC_CPU_MX6SX))
+             max_ecc_strength_supported = 62;
+     else
+             max_ecc_strength_supported = 40;

arm: + mx28evk_nand
+drivers/mtd/nand/mxs_nand.c:155:18: error: 'MXC_CPU_MX6SX' undeclared (first use in this function) +drivers/mtd/nand/mxs_nand.c:155:18: note: each undeclared identifier is reported only once for each function it appears in

Review

Quoted 3 lines
(quoted diff) +3 -0
+make[2]: *** [drivers/mtd/nand/mxs_nand.o] Error 1
+make[1]: *** [drivers/mtd/nand] Error 2
+make: *** [sub-make] Error 2

My bad. I only take mx6 into consideration when I did this patch.

Quoted 5 lines

(among other failed targets)

I tried to fix it by including asm/arch-imx/cpu.h, but then got undefined reference to is_cpu_type().

Now is_cpu_type() is common to all i.MXes, but need this patch
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commitdiff;h=bf3303c98b23a29d99acc5e28865db604873b699
which is still in imx tree now.
After applying the following patches, is_cpu_type should work.
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=7c015efa7f28911d538ddd7d12926edd95176791
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=8b647df9cae0ca65656cca19d4bd43239a9eca42
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=3e26a2224a828cd07d1d1df46cbea7e99cc87993
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=11caa3fa9dae79864726fcb763f100b4fd713884
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=bf3303c98b23a29d99acc5e28865db604873b699

Quoted 6 lines

Then do you need me to repost this patch after the upper five patches in imx tree merge into uboot master branch, or the 3 patches in this patchset go through imx tree?
https://patchwork.ozlabs.org/patch/498048/
https://patchwork.ozlabs.org/patch/498049/
https://patchwork.ozlabs.org/patch/498050/

Acked-by Scott Wood <[email protected]>

I already have patches 1 and 3 applied locally, and will probably send a pull request as soon as buildman finishes. Let me know if you want me to send that, and then you can send patch 2 through imx, or if you want me to pull patches 1 and 3 out so all can go via imx.

-Scott

#6 lore raw

PATCH v2 2/3 mtd: nand: mxs check maximum ecc that platfrom supports

Quoted 70 lines · Scott Wood
Quoted 62 lines · Peng Fan

Hi Scott,

Quoted 37 lines · Scott Wood
Quoted 20 lines · Peng Fan

Check maximum ecc strength for each platfrom to avoid the calculated ecc exceed the limitation.

Signed-off-by Peng Fan <[email protected]>
Signed-off-by Han Xu <[email protected]>
Reviewed-by Marek Vasut <[email protected]>

---

Changes v2:
Add Marek's reviewed by.

 drivers/mtd/nand/mxs_nand.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +0 -0
index 33ce817..97011b2 100644
@@ -149,6 +149,13 @@ static inline uint32_t 
 mxs_nand_get_ecc_strength(uint32_t page_data_size,

drivers/mtd/nand/mxs_nand.c static inline uint32_t · line 159

Quoted 10 lines
drivers/mtd/nand/mxs_nand.c +7 -0
                                              uint32_t page_oob_size)
 {
      int ecc_strength;
+     int max_ecc_strength_supported;
+
+     /* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+     if (is_cpu_type(MXC_CPU_MX6SX))
+             max_ecc_strength_supported = 62;
+     else
+             max_ecc_strength_supported = 40;

arm: + mx28evk_nand
+drivers/mtd/nand/mxs_nand.c:155:18: error: 'MXC_CPU_MX6SX' undeclared (first use in this function) +drivers/mtd/nand/mxs_nand.c:155:18: note: each undeclared identifier is reported only once for each function it appears in

Review

Quoted 3 lines
(quoted diff) +3 -0
+make[2]: *** [drivers/mtd/nand/mxs_nand.o] Error 1
+make[1]: *** [drivers/mtd/nand] Error 2
+make: *** [sub-make] Error 2

My bad. I only take mx6 into consideration when I did this patch.

Quoted 5 lines

(among other failed targets)

I tried to fix it by including asm/arch-imx/cpu.h, but then got undefined reference to is_cpu_type().

Now is_cpu_type() is common to all i.MXes, but need this patch
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commitdiff;h=bf3303c98b23a29d99acc5e28865db604873b699
which is still in imx tree now.
After applying the following patches, is_cpu_type should work.
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=7c015efa7f28911d538ddd7d12926edd95176791
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=8b647df9cae0ca65656cca19d4bd43239a9eca42
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=3e26a2224a828cd07d1d1df46cbea7e99cc87993
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=11caa3fa9dae79864726fcb763f100b4fd713884
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=bf3303c98b23a29d99acc5e28865db604873b699

Quoted 6 lines

Then do you need me to repost this patch after the upper five patches in imx tree merge into uboot master branch, or the 3 patches in this patchset go through imx tree?
https://patchwork.ozlabs.org/patch/498048/
https://patchwork.ozlabs.org/patch/498049/
https://patchwork.ozlabs.org/patch/498050/

Quoted 6 lines
Acked-by Scott Wood <[email protected]>

I already have patches 1 and 3 applied locally, and will probably send a pull request as soon as buildman finishes. Let me know if you want me to send that, and then you can send patch 2 through imx, or if you want me to pull patches 1 and 3 out so all can go via imx.

You can send pull request for patch 1 and 3. The 3 patches are independent on each other.
I'll repost patch 2 when the is_cpu_type patch set merged to uboot upstream master branch with your Acked-by. Then imx or nand tree, both are ok.

Regards,
Peng.

Quoted 3 lines

-Scott

#7 lore raw

PATCH v2 2/3 mtd: nand: mxs check maximum ecc that platfrom supports

Quoted 73 lines · Peng Fan
Quoted 72 lines · Scott Wood
Quoted 62 lines · Peng Fan

Hi Scott,

Quoted 37 lines · Scott Wood
Quoted 20 lines · Peng Fan

Check maximum ecc strength for each platfrom to avoid the calculated ecc exceed the limitation.

Signed-off-by Peng Fan <[email protected]>
Signed-off-by Han Xu <[email protected]>
Reviewed-by Marek Vasut <[email protected]>

---

Changes v2:
Add Marek's reviewed by.

 drivers/mtd/nand/mxs_nand.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +0 -0
index 33ce817..97011b2 100644
@@ -149,6 +149,13 @@ static inline uint32_t 
 mxs_nand_get_ecc_strength(uint32_t page_data_size,

drivers/mtd/nand/mxs_nand.c static inline uint32_t · line 159

Quoted 10 lines
drivers/mtd/nand/mxs_nand.c +7 -0
                                              uint32_t page_oob_size)
 {
      int ecc_strength;
+     int max_ecc_strength_supported;
+
+     /* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+     if (is_cpu_type(MXC_CPU_MX6SX))
+             max_ecc_strength_supported = 62;
+     else
+             max_ecc_strength_supported = 40;

arm: + mx28evk_nand
+drivers/mtd/nand/mxs_nand.c:155:18: error: 'MXC_CPU_MX6SX' undeclared (first use in this function) +drivers/mtd/nand/mxs_nand.c:155:18: note: each undeclared identifier is reported only once for each function it appears in

Review

Quoted 3 lines
(quoted diff) +3 -0
+make[2]: *** [drivers/mtd/nand/mxs_nand.o] Error 1
+make[1]: *** [drivers/mtd/nand] Error 2
+make: *** [sub-make] Error 2

My bad. I only take mx6 into consideration when I did this patch.

Quoted 5 lines

(among other failed targets)

I tried to fix it by including asm/arch-imx/cpu.h, but then got undefined reference to is_cpu_type().

Now is_cpu_type() is common to all i.MXes, but need this patch
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commitdiff;h=bf3303c98b23a29d99acc5e28865db604873b699
which is still in imx tree now.
After applying the following patches, is_cpu_type should work.
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=7c015efa7f28911d538ddd7d12926edd95176791
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=8b647df9cae0ca65656cca19d4bd43239a9eca42
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=3e26a2224a828cd07d1d1df46cbea7e99cc87993
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=11caa3fa9dae79864726fcb763f100b4fd713884
http://git.denx.de/?p=u-boot/u-boot-imx.git;a=commit;h=bf3303c98b23a29d99acc5e28865db604873b699

Quoted 6 lines

Then do you need me to repost this patch after the upper five patches in imx tree merge into uboot master branch, or the 3 patches in this patchset go through imx tree?
https://patchwork.ozlabs.org/patch/498048/
https://patchwork.ozlabs.org/patch/498049/
https://patchwork.ozlabs.org/patch/498050/

Acked-by Scott Wood <[email protected]>
Quoted 7 lines
Quoted 4 lines

I already have patches 1 and 3 applied locally, and will probably send a pull request as soon as buildman finishes. Let me know if you want me to send that, and then you can send patch 2 through imx, or if you want me to pull patches 1 and 3 out so all can go via imx.

You can send pull request for patch 1 and 3. The 3 patches are independent on each other.
I'll repost patch 2 when the is_cpu_type patch set merged to uboot upstream master branch with your Acked-by. Then imx or nand tree, both are ok.

Fine with me.

Regards,
Stefano

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================
author
#8 lore raw

PATCH v2 3/3 mtd: nand: mxs invalidate dcache before DMA read

In reply to Peng Fan

Follow linux dma flow:
Before DMA read, be sure to invalidate the cache over the address range of DMA buffer to prevent cache coherency problems. After DMA read, invalidate dcache again.

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

---

Changes v2:
Add Marek's Acked by

 drivers/mtd/nand/mxs_nand.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/nand/mxs_nand.c +6 -0
index 97011b2..f15cf36 100644
@@ -469,6 +469,9 @@ static void mxs_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int length)
 
 	mxs_dma_desc_append(channel, d);
 
+	/* Invalidate caches */
+	mxs_nand_inval_data_buf(nand_info);
+
 	/* Execute the DMA chain. */
 	ret = mxs_dma_go(channel);
 	if (ret) {

@@ -635,6 +638,9 @@ static int mxs_nand_ecc_read_page(struct mtd_info *mtd, struct nand_chip *nand,
 
 	mxs_dma_desc_append(channel, d);
 
+	/* Invalidate caches */
+	mxs_nand_inval_data_buf(nand_info);
+
 	/* Execute the DMA chain. */
 	ret = mxs_dma_go(channel);
 	if (ret) {
Signature · 1 line
1.8.4
#9 lore raw

PATCH v2 3/3 mtd: nand: mxs invalidate dcache before DMA read

Quoted 11 lines · Peng Fan

Follow linux dma flow:
Before DMA read, be sure to invalidate the cache over the address range of DMA buffer to prevent cache coherency problems. After DMA read, invalidate dcache again.

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

---

Changes v2:
Add Marek's Acked by

You really don't have to repost patches just because an ACK was added ;-) Patchwork collects those, so don't worry about them getting lost.

Best regards,
Marek Vasut

#10 lore raw

PATCH v2 3/3 mtd: nand: mxs invalidate dcache before DMA read

In reply to Peng Fan
Quoted 40 lines · Peng Fan

Follow linux dma flow:
Before DMA read, be sure to invalidate the cache over the address range of DMA buffer to prevent cache coherency problems. After DMA read, invalidate dcache again.

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

---

Changes v2:
Add Marek's Acked by

 drivers/mtd/nand/mxs_nand.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/nand/mxs_nand.c +6 -1
index 97011b2..f15cf36 100644
@@ -469,6 +469,9 @@ static void mxs_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int length)
 
        mxs_dma_desc_append(channel, d);
 
+       /* Invalidate caches */
+       mxs_nand_inval_data_buf(nand_info);
+
        /* Execute the DMA chain. */
        ret = mxs_dma_go(channel);
        if (ret) {

@@ -635,6 +638,9 @@ static int mxs_nand_ecc_read_page(struct mtd_info *mtd, struct nand_chip *nand,
 
        mxs_dma_desc_append(channel, d);
 
+       /* Invalidate caches */
+       mxs_nand_inval_data_buf(nand_info);
+
        /* Execute the DMA chain. */
        ret = mxs_dma_go(channel);
        if (ret) {
--
Quoted 2 lines

1.8.4

Acked-by Tim Harvey <[email protected]>

This resolves an issue I've been digging into regarding IMX6 NAND read failures.

Tim

#11 lore raw

PATCH v2 3/3 mtd: nand: mxs invalidate dcache before DMA read

Quoted 46 lines · Tim Harvey
Quoted 45 lines · Peng Fan

Follow linux dma flow:
Before DMA read, be sure to invalidate the cache over the address range of DMA buffer to prevent cache coherency problems. After DMA read, invalidate dcache again.

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

---

Changes v2:
Add Marek's Acked by

 drivers/mtd/nand/mxs_nand.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/nand/mxs_nand.c +3 -0
index 97011b2..f15cf36 100644
@@ -469,6 +469,9 @@ static void mxs_nand_read_buf(struct mtd_info *mtd,
 uint8_t *buf, int length)
 
        mxs_dma_desc_append(channel, d);
 
+       /* Invalidate caches */
+       mxs_nand_inval_data_buf(nand_info);
+
 
        /* Execute the DMA chain. */

        ret = mxs_dma_go(channel);
        if (ret) {
drivers/mtd/nand/mxs_nand.c +3 -0
@@ -635,6 +638,9 @@ static int mxs_nand_ecc_read_page(struct mtd_info
 *mtd, struct nand_chip *nand,
 
        mxs_dma_desc_append(channel, d);
 
+       /* Invalidate caches */
+       mxs_nand_inval_data_buf(nand_info);
+
 
        /* Execute the DMA chain. */

        ret = mxs_dma_go(channel);
        if (ret) {
Quoted 7 lines
Quoted 2 lines

--
1.8.4

Acked-by Tim Harvey <[email protected]>

This resolves an issue I've been digging into regarding IMX6 NAND read failures.

Indeed, this should be applied ASAP.

Best regards,
Marek Vasut

#12 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

In reply to Peng Fan
Quoted 158 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there is a chance that block_mark_bit_offset conflicts with bch ecc area.

The following graph is modified from kernel gpmi-nand.c driver with each data block 512 bytes. We can see that Block Mark conflicts with ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

   |                          P                                        |
   |<----------------------------------------------------------------->|
   |                                                                   |
   |                                                (Block Mark)       |
   |                      P'                             |           | |   |
   |<--------------------------------------------------->|     D     | | O'|
   |                                                     |<--------->| |<->|
   V                                                     V           V V   V
   +---+--------------+-+--------------+-+--------------+-+----------+-+---+
   | M |   data       |E|   data       |E|   data       |E|   data   |E|   |
   +---+--------------+-+--------------+-+--------------+-+----------+-+---+
                                                        ^                  ^
                                                        |         O        |
                                                        |<---------------->|

P : the page size for BCH module.
E : The ECC strength.
G : the length of Galois Field.
N : The chunk count of per page.
M : the metasize of per page.
C : the ecc chunk size, aka the "data" above. P': the nand chip's page size.
O : the nand chip's oob size.
O': the free oob.

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

---

Changes v2:
none

Changes v1:
The previous patch is https://patchwork.ozlabs.org/patch/422757/
This version contains a minor change to the name from gf_len to galois_field. Also add Marek's Reviewed-by.

 arch/arm/include/asm/imx-common/regs-bch.h |  2 ++
 drivers/mtd/nand/mxs_nand.c                | 32 +++++++++++++++++++++---------
 2 files changed, 25 insertions(+), 9 deletions(-)
arch/arm/include/asm/imx-common/regs-bch.h +2 -0
index a33d341..5c47783 100644
@@ -148,6 +148,7 @@ struct mxs_bch_regs {
 #define        BCH_FLASHLAYOUT0_ECC0_ECC30                     (0xf << 12)
 #define        BCH_FLASHLAYOUT0_ECC0_ECC32                     (0x10 << 12)
 #define        BCH_FLASHLAYOUT0_GF13_0_GF14_1                  (1 << 10)
+#define        BCH_FLASHLAYOUT0_GF13_0_GF14_1_OFFSET           10
 #define        BCH_FLASHLAYOUT0_DATA0_SIZE_MASK                0xfff
 #define        BCH_FLASHLAYOUT0_DATA0_SIZE_OFFSET              0
 

@@ -178,6 +179,7 @@ struct mxs_bch_regs {
 #define        BCH_FLASHLAYOUT1_ECCN_ECC30                     (0xf << 12)
 #define        BCH_FLASHLAYOUT1_ECCN_ECC32                     (0x10 << 12)
 #define        BCH_FLASHLAYOUT1_GF13_0_GF14_1                  (1 << 10)
+#define        BCH_FLASHLAYOUT1_GF13_0_GF14_1_OFFSET           10
 #define        BCH_FLASHLAYOUT1_DATAN_SIZE_MASK                0xfff
 #define        BCH_FLASHLAYOUT1_DATAN_SIZE_OFFSET              0
drivers/mtd/nand/mxs_nand.c +23 -10
index 9c144a2..33ce817 100644
@@ -68,6 +68,8 @@ struct mxs_nand_info {
 };
 
 struct nand_ecclayout fake_ecc_layout;
+static int chunk_data_size = MXS_NAND_CHUNK_DATA_CHUNK_SIZE;
+static int galois_field = 13;
 
 /*
  * Cache management functions

@@ -130,12 +132,12 @@ static void mxs_nand_return_dma_descs(struct mxs_nand_info *info)
 
 static uint32_t mxs_nand_ecc_chunk_cnt(uint32_t page_data_size)
 {
-       return page_data_size / MXS_NAND_CHUNK_DATA_CHUNK_SIZE;
+       return page_data_size / chunk_data_size;
 }
 
 static uint32_t mxs_nand_ecc_size_in_bits(uint32_t ecc_strength)
 {
-       return ecc_strength * MXS_NAND_BITS_PER_ECC_LEVEL;
+       return ecc_strength * galois_field;
 }
 
 static uint32_t mxs_nand_aux_status_offset(void)

@@ -157,8 +159,8 @@ static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
         *              (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));
+                       / (galois_field *
+                          mxs_nand_ecc_chunk_cnt(page_data_size));
 
        return round_down(ecc_strength, 2);
 }

@@ -173,7 +175,7 @@ static inline uint32_t mxs_nand_get_mark_offset(uint32_t page_data_size,
        uint32_t block_mark_chunk_bit_offset;
        uint32_t block_mark_bit_offset;
 
-       chunk_data_size_in_bits = MXS_NAND_CHUNK_DATA_CHUNK_SIZE * 8;
+       chunk_data_size_in_bits = chunk_data_size * 8;
        chunk_ecc_size_in_bits  = mxs_nand_ecc_size_in_bits(ecc_strength);
 
        chunk_total_size_in_bits =

@@ -972,6 +974,16 @@ static int mxs_nand_scan_bbt(struct mtd_info *mtd)
        struct mxs_bch_regs *bch_regs = (struct mxs_bch_regs *)MXS_BCH_BASE;
        uint32_t tmp;
 
+       if (mtd->oobsize > MXS_NAND_CHUNK_DATA_CHUNK_SIZE) {
+               galois_field = 14;
+               chunk_data_size = MXS_NAND_CHUNK_DATA_CHUNK_SIZE * 2;
+       }
+
+       if (mtd->oobsize > chunk_data_size) {
+               printf("Not support the NAND chips whose oob size is larger then %d bytes!\n", chunk_data_size);
+               return -EINVAL;
+       }
+
        /* Configure BCH and set NFC geometry */
        mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
 

@@ -981,16 +993,18 @@ static int mxs_nand_scan_bbt(struct mtd_info *mtd)
        tmp |= MXS_NAND_METADATA_SIZE << BCH_FLASHLAYOUT0_META_SIZE_OFFSET;
        tmp |= (mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize) >> 1)
                << BCH_FLASHLAYOUT0_ECC0_OFFSET;
-       tmp |= MXS_NAND_CHUNK_DATA_CHUNK_SIZE
-               >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
+       tmp |= chunk_data_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
+       tmp |= (14 == galois_field ? 1 : 0) <<
+               BCH_FLASHLAYOUT0_GF13_0_GF14_1_OFFSET;
        writel(tmp, &bch_regs->hw_bch_flash0layout0);
 
        tmp = (mtd->writesize + mtd->oobsize)
                << BCH_FLASHLAYOUT1_PAGE_SIZE_OFFSET;
        tmp |= (mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize) >> 1)
                << BCH_FLASHLAYOUT1_ECCN_OFFSET;
-       tmp |= MXS_NAND_CHUNK_DATA_CHUNK_SIZE
-               >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
+       tmp |= chunk_data_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
+       tmp |= (14 == galois_field ? 1 : 0) <<
+               BCH_FLASHLAYOUT1_GF13_0_GF14_1_OFFSET;
        writel(tmp, &bch_regs->hw_bch_flash0layout1);
 
        /* Set *all* chip selects to use layout 0 */
--
Quoted 2 lines

1.8.4

Tested on IMX6 with MT29F16G08 16Gb and MT29F2G08 2Gb NAND devices.

Tested-by Tim Harvey <[email protected]>
#13 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

In reply to Peng Fan
Quoted 7 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there is a chance that block_mark_bit_offset conflicts with bch ecc area.

The following graph is modified from kernel gpmi-nand.c driver with each data block 512 bytes. We can see that Block Mark conflicts with ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Enlarge it by how much? What does the layout look like in that case?

Quoted 16 lines
   |                          P                                        |
   |<----------------------------------------------------------------->|
   |                                                                   |
   |                                                (Block Mark)       |
   |                      P'                             |           | |   |
   |<--------------------------------------------------->|     D     | | O'|
   |                                                     |<--------->| |<->|
   V                                                     V           V V   V
   +---+--------------+-+--------------+-+--------------+-+----------+-+---+
   | M |   data       |E|   data       |E|   data       |E|   data   |E|   |
   +---+--------------+-+--------------+-+--------------+-+----------+-+---+
                                                        ^                  ^
                                                        |         O        |
                                                        |<---------------->|
Quoted 9 lines

P : the page size for BCH module.
E : The ECC strength.
G : the length of Galois Field.
N : The chunk count of per page.
M : the metasize of per page.
C : the ecc chunk size, aka the "data" above. P': the nand chip's page size.
O : the nand chip's oob size.
O': the free oob.

"D" is in the diagram but not the key. "G", "N", and "C" are in the key but not the diagram.

What is "the metasize of per page"?

-Scott

#14 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Quoted 10 lines · Scott Wood
Quoted 7 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there is a chance that block_mark_bit_offset conflicts with bch ecc area.

The following graph is modified from kernel gpmi-nand.c driver with each data block 512 bytes. We can see that Block Mark conflicts with ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Enlarge it by how much? What does the layout look like in that case?

Enlarge it to 1024 bytes.
In normal case the Block Mark bit should locates at the data part, but not at the ECC area.
You can take the following as an example.

   |                          P                                    |
   |<------------------------------------------------------------->|
   |                                                               |
   |                                          (Block Mark)         |
   |                      P'                         |           | |   |
   |<----------------------------------------------->|     D     | | O'|
   |                                                 |<--------->| |<->|
   V                                                 V           V V   V
   +---+---------------------------+-+---------------------------+-+---+
   | M |   data                    |E|   data             data   |E|   |
   +---+---------------------------+-+---------------------------+-+---+
                                                    ^                  ^
                                                    |         O        |
                                                    |<---------------->|
Quoted 27 lines
Quoted 17 lines

   |                          P                                        |
   |<----------------------------------------------------------------->|
   |                                                                   |
   |                                                (Block Mark)       |
   |                      P'                             |           | |   |
   |<--------------------------------------------------->|     D     | | O'|
   |                                                     |<--------->| |<->|
   V                                                     V           V V   V
   +---+--------------+-+--------------+-+--------------+-+----------+-+---+
   | M |   data       |E|   data       |E|   data       |E|   data   |E|   |
   +---+--------------+-+--------------+-+--------------+-+----------+-+---+
                                                        ^                  ^
                                                        |         O        |
                                                        |<---------------->|
Quoted 9 lines

P : the page size for BCH module.
E : The ECC strength.
G : the length of Galois Field.
N : The chunk count of per page.
M : the metasize of per page.
C : the ecc chunk size, aka the "data" above. P': the nand chip's page size.
O : the nand chip's oob size.
O': the free oob.

Quoted 2 lines

"D" is in the diagram but not the key. "G", "N", and "C" are in the key but not the diagram.

C is ecc chunk data size, same the data area. Here it is 512 bytes.
N is chunk count. for exmaple page size is 2048 bytes, ecc chunk data size is 512 bytes, then N is 2048/512. G is the Galois Field, its value is 13 or 14. The should be got from reference mannual.
D is in the digram , but not the key. I think it is enougth to show why this patch is done.

I put the digram here to show that Block Mark may conflicts with ECC area, and this is wrong, we need to find a way to avoid this. And the way is to enlarge the ecc chunk data size from 512 to 1024 bytes.

I did not put the formula here. The keys is same to keys in kernel gpmi-nand.c.

The formula for P is :

E * G * N
P = ------------ + P' + M
8

The position of block mark moves forward in the ECC-based view of page, and the delta is:

                   E * G * (N - 1)
             D = (---------------- + M)
                          8

I think there is no need to put the formula in commit msg. Also, to explain D is not relevant to this patch.

Quoted 2 lines

What is "the metasize of per page"?

10 bytes.

Peng.

Quoted 3 lines

-Scott

#15 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Quoted 5 lines · Peng Fan
Quoted 4 lines · Scott Wood
Quoted 3 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there is a chance that block_mark_bit_offset conflicts with bch ecc area.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

The following graph is modified from kernel gpmi-nand.c driver with each data block 512 bytes. We can see that Block Mark conflicts with ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Enlarge it by how much? What does the layout look like in that case?

Enlarge it to 1024 bytes.

Then say so in the changelog.

Quoted 2 lines

In normal case the Block Mark bit should locates at the data part, but not at the ECC area.

It seems like either would be bad...

Quoted 82 lines

You can take the following as an example.

   |                          P                                    |
   |<------------------------------------------------------------->|
   |                                                               |
   |                                          (Block Mark)         |
   |                      P'                         |           | |   |
   |<----------------------------------------------->|     D     | | O'|
   |                                                 |<--------->| |<->|
   V                                                 V           V V   V
   +---+---------------------------+-+---------------------------+-+---+
   | M |   data                    |E|   data             data   |E|   |
   +---+---------------------------+-+---------------------------+-+---+
                                                    ^                  ^
                                                    |         O        |
                                                    |<---------------->|
Quoted 37 lines
Quoted 27 lines

   |                          P                                        |
   |<----------------------------------------------------------------->|
   |                                                                   |
   |                                                (Block Mark)       |
   |                      P'                             |           | |
   |
   |<--------------------------------------------------->|     D     | |
O'|
   |                                                     |<--------->| 
|<->|
   V                                                     V           V V
   V
   +---+--------------+-+--------------+-+--------------+-+----------+-+
---+
   | M |   data       |E|   data       |E|   data       |E|   data   
|E|   |
   +---+--------------+-+--------------+-+--------------+-+----------+-+
---+
                                                        ^               
   ^
                                                        |         O     
   |
                                                        |<--------------

-->|

Quoted 9 lines

P : the page size for BCH module.
E : The ECC strength.
G : the length of Galois Field.
N : The chunk count of per page.
M : the metasize of per page.
C : the ecc chunk size, aka the "data" above. P': the nand chip's page size.
O : the nand chip's oob size.
O': the free oob.

Quoted 3 lines

"D" is in the diagram but not the key. "G", "N", and "C" are in the key but
not the diagram.

C is ecc chunk data size, same the data area. Here it is 512 bytes. N is chunk count. for exmaple page size is 2048 bytes, ecc chunk data size is 512 bytes, then N is 2048/512.
G is the Galois Field, its value is 13 or 14. The should be got from reference mannual.
D is in the digram , but not the key. I think it is enougth to show why this patch is done.

I put the digram here to show that Block Mark may conflicts with ECC area, and this is wrong, we need to find a way to avoid this. And the way is to enlarge the ecc chunk data size from 512 to 1024 bytes.

I did not put the formula here. The keys is same to keys in kernel gpmi- nand.c.

The formula for P is :

E * G * N
P = ------------ + P' + M
8

The position of block mark moves forward in the ECC-based view of page, and the delta is:

Quoted 6 lines
                   E * G * (N - 1)
             D = (---------------- + M)
                          8

I think there is no need to put the formula in commit msg. Also, to explain D is not relevant to this patch.

I'm not asking for the above to go into the commit message. I'm asking for what does go in the commit message to be less cryptic.

Quoted 3 lines
Quoted 1 line

What is "the metasize of per page"?

10 bytes.

But what does it mean?

-Scott

#16 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Quoted 11 lines · Scott Wood
Quoted 10 lines · Peng Fan
Quoted 9 lines · Scott Wood
Quoted 7 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there is a chance that block_mark_bit_offset conflicts with bch ecc area.

The following graph is modified from kernel gpmi-nand.c driver with each data block 512 bytes. We can see that Block Mark conflicts with ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

Enlarge it by how much? What does the layout look like in that case?

Enlarge it to 1024 bytes.

Then say so in the changelog.

You mean I need to add this in commit msg and send out a new patch version? Or you pick this one?

Quoted 5 lines
Quoted 3 lines

In normal case the Block Mark bit should locates at the data part, but not at the ECC area.

It seems like either would be bad...

No. To read/write with ecc enabled, the block mark will be swapped with byte in metadata area. This is correct way for GPMI + BCH to perform nand reading/writing with ecc enabled. See following:
645 /* Read DMA completed, now do the mark swapping. */
646 mxs_nand_swap_block_mark(mtd, nand_info->data_buf, nand_info->oob_buf);

When GPMI + BCH used for DMA read, meta data will be stored to oob_buf. Then swap the data at Block Mark with data at oob_buf[0](actually metadata[0]).

Before DMA write, Block Mark will be swapped with metadata[0]. Then DMA write. Then Block Mark should not conlicts with ECC area, but ok with DATA area. Anyway Block Mark should be aways points to bit at a data block.

Quoted 90 lines
Quoted 83 lines

You can take the following as an example.

   |                          P                                    |
   |<------------------------------------------------------------->|
   |                                                               |
   |                                          (Block Mark)         |
   |                      P'                         |           | |   |
   |<----------------------------------------------->|     D     | | O'|
   |                                                 |<--------->| |<->|
   V                                                 V           V V   V
   +---+---------------------------+-+---------------------------+-+---+
   | M |   data                    |E|   data             data   |E|   |
   +---+---------------------------+-+---------------------------+-+---+
                                                    ^                  ^
                                                    |         O        |
                                                    |<---------------->|
Quoted 37 lines
Quoted 27 lines

   |                          P                                        |
   |<----------------------------------------------------------------->|
   |                                                                   |
   |                                                (Block Mark)       |
   |                      P'                             |           | |
   |
   |<--------------------------------------------------->|     D     | |
O'|
   |                                                     |<--------->| 
|<->|
   V                                                     V           V V
   V
   +---+--------------+-+--------------+-+--------------+-+----------+-+
---+
   | M |   data       |E|   data       |E|   data       |E|   data   
|E|   |
   +---+--------------+-+--------------+-+--------------+-+----------+-+
---+
                                                        ^               
   ^
                                                        |         O     
   |
                                                        |<--------------

-->|

Quoted 9 lines

P : the page size for BCH module.
E : The ECC strength.
G : the length of Galois Field.
N : The chunk count of per page.
M : the metasize of per page.
C : the ecc chunk size, aka the "data" above. P': the nand chip's page size.
O : the nand chip's oob size.
O': the free oob.

Quoted 3 lines

"D" is in the diagram but not the key. "G", "N", and "C" are in the key but
not the diagram.

C is ecc chunk data size, same the data area. Here it is 512 bytes. N is chunk count. for exmaple page size is 2048 bytes, ecc chunk data size is 512 bytes, then N is 2048/512.
G is the Galois Field, its value is 13 or 14. The should be got from reference mannual.
D is in the digram , but not the key. I think it is enougth to show why this patch is done.

I put the digram here to show that Block Mark may conflicts with ECC area, and this is wrong, we need to find a way to avoid this. And the way is to enlarge the ecc chunk data size from 512 to 1024 bytes.

I did not put the formula here. The keys is same to keys in kernel gpmi- nand.c.

The formula for P is :

E * G * N
P = ------------ + P' + M
8

The position of block mark moves forward in the ECC-based view of page, and the delta is:

Quoted 6 lines
                   E * G * (N - 1)
             D = (---------------- + M)
                          8

I think there is no need to put the formula in commit msg. Also, to explain D is not relevant to this patch.

Quoted 8 lines

I'm not asking for the above to go into the commit message. I'm asking for what does go in the commit message to be less cryptic.

Quoted 3 lines
Quoted 1 line

What is "the metasize of per page"?

10 bytes.

But what does it mean?

BCH layout for data stored in nand page:
Meta + DATA + ECC + DATA + ECC + DATA + ECC + DATA + ECC

After usind DMA read, the layout in buffer is following:
DATA + DATA + DATA + DATA | META + status of each data block
stored in data buffer | stored in oob buffer

Hope this is clear.

Peng.

Quoted 3 lines

-Scott

#17 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Quoted 12 lines · Peng Fan
Quoted 11 lines · Scott Wood
Quoted 10 lines · Peng Fan
Quoted 9 lines · Scott Wood
Quoted 7 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there is a chance that block_mark_bit_offset conflicts with bch ecc area.

The following graph is modified from kernel gpmi-nand.c driver with each data block 512 bytes. We can see that Block Mark conflicts with ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

Enlarge it by how much? What does the layout look like in that case?

Enlarge it to 1024 bytes.

Then say so in the changelog.

You mean I need to add this in commit msg and send out a new patch version? Or you pick this one?

This discussion is becoming ridiculous, can we please get this bugfix applied ? If you don't like some minor details in the commit message, can you please fix them while applying ?

Thanks

[...]
Best regards,
Marek Vasut

#18 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Quoted 28 lines · Marek Vasut
Quoted 19 lines · Peng Fan
Quoted 18 lines · Scott Wood
Quoted 17 lines · Peng Fan
Quoted 15 lines · Scott Wood
Quoted 11 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there
is a chance that block_mark_bit_offset conflicts with bch ecc area.

The following graph is modified from kernel gpmi-nand.c driver with
each data block 512 bytes. We can see that Block Mark conflicts with
ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Enlarge it by how much? What does the layout look like in that case?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

Enlarge it to 1024 bytes.

Then say so in the changelog.

You mean I need to add this in commit msg and send out a new patch version?
Or you pick this one?

Quoted 5 lines

This discussion is becoming ridiculous, can we please get this bugfix applied ?
If you don't like some minor details in the commit message, can you please fix
them while applying ?

Yes, I can edit the changelog while applying, but that doesn't mean I'm not going to complain about a difficult-to-understand changelog, and I still would like to understand what is actually going on here. Don't assume I'm familiar with this hardware or its unusual page layout. You can help by explaining things, or you can not help by throwing a fit...

-Scott

#19 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Quoted 35 lines · Scott Wood
Quoted 28 lines · Marek Vasut
Quoted 19 lines · Peng Fan
Quoted 18 lines · Scott Wood
Quoted 17 lines · Peng Fan
Quoted 15 lines · Scott Wood
Quoted 11 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there
is a chance that block_mark_bit_offset conflicts with bch ecc area.

The following graph is modified from kernel gpmi-nand.c driver with
each data block 512 bytes. We can see that Block Mark conflicts with
ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Enlarge it by how much? What does the layout look like in that case?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

Enlarge it to 1024 bytes.

Then say so in the changelog.

You mean I need to add this in commit msg and send out a new patch version?
Or you pick this one?

Quoted 5 lines

This discussion is becoming ridiculous, can we please get this bugfix applied ?
If you don't like some minor details in the commit message, can you please fix
them while applying ?

Quoted 5 lines

Yes, I can edit the changelog while applying, but that doesn't mean I'm not going to complain about a difficult-to-understand changelog, and I still would like to understand what is actually going on here. Don't assume I'm familiar with this hardware or its unusual page layout. You can help by explaining things, or you can not help by throwing a fit...

I can point you to MX28 datasheet [1] chapter 16.2.2 and onward if you want to educate yourself, it's all explained there, concisely and clearly.

[1] http://free-electrons.com/~maxime/pub/datasheet/MCIMX28RM.pdf

Best regards,
Marek Vasut

#20 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Quoted 44 lines · Marek Vasut
Quoted 36 lines · Scott Wood
Quoted 29 lines · Marek Vasut
Quoted 20 lines · Peng Fan
Quoted 19 lines · Scott Wood
Quoted 18 lines · Peng Fan
Quoted 16 lines · Scott Wood
Quoted 5 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there
is a chance that block_mark_bit_offset conflicts with bch ecc area.

Quoted 7 lines

The following graph is modified from kernel gpmi-nand.c driver with
each data block 512 bytes. We can see that Block Mark conflicts
with
ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Enlarge it by how much? What does the layout look like in that case?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

Enlarge it to 1024 bytes.

Then say so in the changelog.

You mean I need to add this in commit msg and send out a new patch version?
Or you pick this one?

Quoted 5 lines

This discussion is becoming ridiculous, can we please get this bugfix applied ?
If you don't like some minor details in the commit message, can you please fix
them while applying ?

Quoted 6 lines

Yes, I can edit the changelog while applying, but that doesn't mean I'm not
going to complain about a difficult-to-understand changelog, and I still would like to understand what is actually going on here. Don't assume I'm familiar with this hardware or its unusual page layout. You can help by explaining things, or you can not help by throwing a fit...

Quoted 4 lines

I can point you to MX28 datasheet [1] chapter 16.2.2 and onward if you want to educate yourself, it's all explained there, concisely and clearly.

[1] http://free-electrons.com/~maxime/pub/datasheet/MCIMX28RM.pdf

Thanks. That preempted a question I was just about to ask Peng, because it wasn't clear that the meta area was covered by ECC.

-Scott

#21 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Quoted 45 lines · Scott Wood
Quoted 44 lines · Marek Vasut
Quoted 42 lines · Scott Wood
Quoted 29 lines · Marek Vasut
Quoted 20 lines · Peng Fan
Quoted 19 lines · Scott Wood
Quoted 18 lines · Peng Fan
Quoted 16 lines · Scott Wood
Quoted 5 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there
is a chance that block_mark_bit_offset conflicts with bch ecc area.

Quoted 7 lines

The following graph is modified from kernel gpmi-nand.c driver with
each data block 512 bytes. We can see that Block Mark conflicts
with
ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Enlarge it by how much? What does the layout look like in that case?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

Enlarge it to 1024 bytes.

Then say so in the changelog.

You mean I need to add this in commit msg and send out a new patch version?
Or you pick this one?

Quoted 5 lines

This discussion is becoming ridiculous, can we please get this bugfix applied ?
If you don't like some minor details in the commit message, can you please fix
them while applying ?

Yes, I can edit the changelog while applying, but that doesn't mean I'm not
going to complain about a difficult-to-understand changelog, and I still would like to understand what is actually going on here. Don't assume I'm familiar with this hardware or its unusual page layout. You can help by explaining things, or you can not help by throwing a fit...

Quoted 7 lines
Quoted 4 lines

I can point you to MX28 datasheet [1] chapter 16.2.2 and onward if you want to educate yourself, it's all explained there, concisely and clearly.

[1] http://free-electrons.com/~maxime/pub/datasheet/MCIMX28RM.pdf

Thanks. That preempted a question I was just about to ask Peng, because it wasn't clear that the meta area was covered by ECC.

In mxs_nand.c driver, we use "Combined Metadata & Block 0, unbalanced ECC coverage" layout from chapter 16.2.2 of MX28 datasheet.

Peng.

Quoted 3 lines

-Scott

#22 lore raw

PATCH RESEND v2 1/3 mtd: nand: mxs support oobsize bigger than 512

Hi Scott,

Do you have plan to pick the 3 patches?

https://patchwork.ozlabs.org/patch/498050/
https://patchwork.ozlabs.org/patch/498049/
https://patchwork.ozlabs.org/patch/498048/

If not, then I prefer these 3 patches can go throught i.mx tree.

Thanks,
Peng.

Quoted 62 lines · Peng Fan
Quoted 45 lines · Scott Wood
Quoted 44 lines · Marek Vasut
Quoted 42 lines · Scott Wood
Quoted 29 lines · Marek Vasut
Quoted 20 lines · Peng Fan
Quoted 19 lines · Scott Wood
Quoted 18 lines · Peng Fan
Quoted 16 lines · Scott Wood
Quoted 5 lines · Peng Fan

If ecc chunk data size is 512 and oobsize is bigger than 512, there
is a chance that block_mark_bit_offset conflicts with bch ecc area.

Quoted 7 lines

The following graph is modified from kernel gpmi-nand.c driver with
each data block 512 bytes. We can see that Block Mark conflicts
with
ecc area from bch view. We can enlarge the ecc chunk size to avoid this problem to those oobsize which is larger than 512.

Enlarge it by how much? What does the layout look like in that case?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

Enlarge it to 1024 bytes.

Then say so in the changelog.

You mean I need to add this in commit msg and send out a new patch version?
Or you pick this one?

Quoted 5 lines

This discussion is becoming ridiculous, can we please get this bugfix applied ?
If you don't like some minor details in the commit message, can you please fix
them while applying ?

Yes, I can edit the changelog while applying, but that doesn't mean I'm not
going to complain about a difficult-to-understand changelog, and I still would like to understand what is actually going on here. Don't assume I'm familiar with this hardware or its unusual page layout. You can help by explaining things, or you can not help by throwing a fit...

Quoted 7 lines
Quoted 4 lines

I can point you to MX28 datasheet [1] chapter 16.2.2 and onward if you want to educate yourself, it's all explained there, concisely and clearly.

[1] http://free-electrons.com/~maxime/pub/datasheet/MCIMX28RM.pdf

Thanks. That preempted a question I was just about to ask Peng, because it wasn't clear that the meta area was covered by ECC.

In mxs_nand.c driver, we use "Combined Metadata & Block 0, unbalanced ECC coverage" layout from chapter 16.2.2 of MX28 datasheet.

Peng.

Quoted 3 lines

-Scott