am335x: NAND: add BCH16 and 4k page size support

PATCH New Jordy van Wolferen 3 participants 3 messages Jan 28, 2013 – Jan 29, 2013

Subsystems (3)

Arm (32-bit) Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Scott Wood

2013-01-29 19:54 UTC #1 lore raw

PATCH 1/1 am335x: NAND: add BCH16 and 4k page size support

Quoted 10 lines · Jordy van Wolferen

This is tested with a custom AM3359 (rev 2.0) board. NAND chip: MT29F16G08ABABAWP
This code allows me to boot from ROM code.
The ROM code forces BCH16 on NAND chips with a 4k page size.
BCH16 is not enabled by default.
---

Missing Signed-off-by (please read the "Sign your work" section of Documentation/SubmittingPatches in Linux and be sure that you meet the conditions of the Developer's Certificate of Origin before adding your sign off).

Could you explain the patch in a bit more detail? You say it is "not enabled by default" -- what would be required to enable it?

Quoted 107 lines

 arch/arm/include/asm/arch-am33xx/cpu.h       |   8 +-
 arch/arm/include/asm/arch-am33xx/omap_gpmc.h |  43 ++++++++
 drivers/mtd/nand/omap_gpmc.c                 | 150  
+++++++++++++++++----------
 include/linux/mtd/mtd-abi.h                  |   2 +-
 4 files changed, 148 insertions(+), 55 deletions(-)
arch/arm/include/asm/arch-am33xx/cpu.h +7 -1
index 16e8a80..0a1f1ff 100644
@@ -78,6 +78,10 @@ struct bch_res_0_3 {
 	u32 bch_result_x[4];
 };
 
+struct bch_res_4_6 {
+	u32 bch_result_x[3];
+};
+
 struct gpmc {
 	u8 res1[0x10];
 	u32 sysconfig;		/* 0x10 */
@@ -107,7 +111,9 @@ struct gpmc {
 	u8 res7[12];		/* 0x224 */
 	u32 testmomde_ctrl;	/* 0x230 */
 	u8 res8[12];		/* 0x234 */
-	struct bch_res_0_3 bch_result_0_3[2];	/* 0x240 */
+	struct bch_res_0_3 bch_result_0_3;	/* 0x240 */
+	u32 dummy[44];		/* not used */
+	struct bch_res_4_6 bch_result_4_6;	/* 300 */
 };
 
 /* Used for board specific gpmc initialization */
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +6 -0
index 572f9d0..534fa6e 100644
@@ -117,4 +117,47 @@
 		{.offset = 106,\
 		 .length = 8 } } \
 }
+
+#define GPMC_NAND_4K_HW_BCH8_ECC_LAYOUT {\
+	.eccbytes = 112,\
+	.eccpos = {2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,\
+				16, 17, 18, 19, 20, 21, 22, 23, 24, 25,  
 26, 27,\
+				28, 29, 30, 31, 32, 33, 34, 35, 36, 37,  
38, 39,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				40, 41, 42, 43, 44, 45, 46, 47, 48, 49,  
50, 51,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				52, 53, 54, 55, 56, 57, 58, 59, 60, 61,  
62, 63,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				64, 65, 66, 67, 68, 69, 70, 71, 72, 73,  
74, 75,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				76, 77, 78, 79, 80, 81, 82, 83, 84, 85,  
86, 87,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				88, 89, 90, 91, 92, 93, 94, 95, 96, 97,  
98, 99,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				100, 101, 102, 103, 104, 105, 106, 107,  
108, 109,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +10 -0
+				110, 111, 112, 113},\
+	.oobfree = {\
+		{.offset = 114,\
+		 .length = 110 } } \
+}
+
+#define GPMC_NAND_4K_HW_BCH16_ECC_LAYOUT {\
+	.eccbytes = 208,\
+	.eccpos = { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,\
+				16, 17, 18, 19, 20, 21, 22, 23, 24, 25,  
26, 27,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				28, 29, 30, 31, 32, 33, 34, 35, 36, 37,  
38, 39,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				40, 41, 42, 43, 44, 45, 46, 47, 48, 49,  
50, 51,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				52, 53, 54, 55, 56, 57, 58, 59, 60, 61,  
62, 63,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				64, 65, 66, 67, 68, 69, 70, 71, 72, 73,  
74, 75,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				76, 77, 78, 79, 80, 81, 82, 83, 84, 85,  
86, 87,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				88, 89, 90, 91, 92, 93, 94, 95, 96, 97,  
98, 99,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				100, 101, 102, 103, 104, 105, 106, 107,  
108, 109,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				110, 111, 112, 113, 114, 115, 116, 117,  
118, 119,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				120, 121, 122, 123, 124, 125, 126, 127,  
128, 129,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				130, 131, 132, 133, 134, 135, 136, 137,  
138, 139,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				140, 141, 142, 143, 144, 145, 146, 147,  
148, 149,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				150, 151, 152, 153, 154, 155, 156, 157,  
158, 159,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				160, 161, 162, 163, 164, 165, 166, 167,  
168, 169,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				170, 171, 172, 173, 174, 175, 176, 177,  
178, 179,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				180, 181, 182, 183, 184, 185, 186, 187,  
188, 189,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				190, 191, 192, 193, 194, 195, 196, 197,  
198, 199,\
arch/arm/include/asm/arch-am33xx/omap_gpmc.h +1 -0
+				200, 201, 202, 203, 204, 205, 206, 207,  

Quoted 1 line

208, 209},\

You have too many tabs here -- be sure your editor is set for 8-character tabs.

Quoted 11 lines

arch/arm/include/asm/arch-am33xx/omap_gpmc.h +4 -0
+	.oobfree = {\
+		{.offset = 210,\
+		 .length = 14 } } \
+}
 #endif /* __ASM_ARCH_OMAP_GPMC_H */
drivers/mtd/nand/omap_gpmc.c +0 -0
index cee394e..3c42a54 100644

drivers/mtd/nand/omap_gpmc.c int omap_spl_dev_ready(struct mtd_info *mtd) · line 83

Quoted 10 lines

drivers/mtd/nand/omap_gpmc.c +2 -2

@@ -76,8 +76,8 @@ int omap_spl_dev_ready(struct mtd_info *mtd)
 
 /*
  * omap_hwecc_init - Initialize the Hardware ECC for NAND flash in
- *                   GPMC controller
- * @mtd:        MTD device structure
+ *					 GPMC controller
+ * @mtd:		MTD device structure
  *
  */
 static void __maybe_unused omap_hwecc_init(struct nand_chip *chip)

No unrelated whitespace changes, please.

drivers/mtd/nand/omap_gpmc.c struct nand_bch_priv { · line 263

Quoted 8 lines

drivers/mtd/nand/omap_gpmc.c +1 -1

@@ -258,7 +258,7 @@ struct nand_bch_priv {
 #define ECC_BCH8_NIBBLES	26
 #define ECC_BCH16_NIBBLES	52
 
-static struct nand_ecclayout hw_bch8_nand_oob =  
 GPMC_NAND_HW_BCH8_ECC_LAYOUT;
+static struct nand_ecclayout nand_ecclayout =  
 GPMC_NAND_HW_BCH8_ECC_LAYOUT;

Why the name change? It's still the bch8 layout.

If your intent is for this to be the configuration knob, U-Boot is not configured by making edits to random source files. It needs to be a proper config symbol (and documented).

Quoted 40 lines

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

 static struct nand_bch_priv bch_priv = {

drivers/mtd/nand/omap_gpmc.c +4 -4

 	.mode = NAND_ECC_HW_BCH,

@@ -280,21 +280,21 @@ static void omap_read_bch8_result(struct  
 mtd_info *mtd, uint8_t big_endian,
 	int8_t i = 0, j;
 
 	if (big_endian) {
-		ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3];
+		ptr = &gpmc_cfg->bch_result_0_3.bch_result_x[3];
 		ecc_code[i++] = readl(ptr) & 0xFF;
 		ptr--;
 		for (j = 0; j < 3; j++) {
 			ecc_code[i++] = (readl(ptr) >> 24) & 0xFF;
 			ecc_code[i++] = (readl(ptr) >> 16) & 0xFF;
-			ecc_code[i++] = (readl(ptr) >>  8) & 0xFF;
+			ecc_code[i++] = (readl(ptr) >>	8) & 0xFF;
 			ecc_code[i++] = readl(ptr) & 0xFF;
 			ptr--;
 		}
 	} else {
-		ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[0];
+		ptr = &gpmc_cfg->bch_result_0_3.bch_result_x[0];
 		for (j = 0; j < 3; j++) {
 			ecc_code[i++] = readl(ptr) & 0xFF;
-			ecc_code[i++] = (readl(ptr) >>  8) & 0xFF;
+			ecc_code[i++] = (readl(ptr) >>	8) & 0xFF;
 			ecc_code[i++] = (readl(ptr) >> 16) & 0xFF;
 			ecc_code[i++] = (readl(ptr) >> 24) & 0xFF;

drivers/mtd/nand/omap_gpmc.c +5 -0

 			ptr++;

@@ -304,6 +304,53 @@ static void omap_read_bch8_result(struct  
 mtd_info *mtd, uint8_t big_endian,
 	}
 }
 
+static void omap_read_bch16_result(struct mtd_info *mtd, uint8_t  
 big_endian,
+				uint8_t *ecc_code)
+{
+	uint32_t *ptr;
+	int8_t i = 0, j;

drivers/mtd/nand/omap_gpmc.c static void omap_read_bch8_result(struct · line 323

Quoted 10 lines

drivers/mtd/nand/omap_gpmc.c +9 -0

+	uint32_t data;
+
+	if(big_endian) {
+		ptr = &gpmc_cfg->bch_result_4_6.bch_result_x[2];
+
+		for (j = 0; j < 7; j++) {
+			if(j == 3) {
+				ptr =  
 &gpmc_cfg->bch_result_0_3.bch_result_x[3];
+			}

Please put a space after "if". Don't use braces around a single line.

Quoted 5 lines

drivers/mtd/nand/omap_gpmc.c +5 -0

+
+			data = readl(ptr);
+			ptr--;
+
+			if(i > 0) {

drivers/mtd/nand/omap_gpmc.c static void omap_read_bch8_result(struct mtd_info *mtd, uint8_t big_endian, · line 335

Quoted 10 lines

drivers/mtd/nand/omap_gpmc.c +10 -0

+				ecc_code[i++] = (data >> 24) & 0xFF;
+				ecc_code[i++] = (data >> 16) & 0xFF;
+			}
+			ecc_code[i++] = (data >> 8) & 0xFF;
+			ecc_code[i++] = data & 0xFF;
+		}
+		ecc_code[i++] = 0;
+		ecc_code[i++] = 0;
+	}
+	else {

} else {

Quoted 33 lines

drivers/mtd/nand/omap_gpmc.c +5 -0

+		ptr = &gpmc_cfg->bch_result_0_3.bch_result_x[0];
+
+		for (j = 0; j < 7; j++) {
+			if(j == 4) {
+				ptr =

&gpmc_cfg->bch_result_4_6.bch_result_x[0];

drivers/mtd/nand/omap_gpmc.c +14 -1

+			}
+
+			data = readl(ptr);
+			ptr++;
+
+			ecc_code[i++] = data & 0xFF;
+			ecc_code[i++] = (data >> 8) & 0xFF;
+			ecc_code[i++] = (data >> 16) & 0xFF;
+			ecc_code[i++] = (data >> 24) & 0xFF;
+		}
+	}
+}
+
 /*
  * omap_ecc_disable - Disable H/W ECC calculation
  *

@@ -330,7 +377,7 @@ static void omap_rotate_ecc_bch(struct mtd_info  
 *mtd, uint8_t *calc_ecc,
 	struct nand_chip *chip = mtd->priv;
 	struct nand_bch_priv *bch = chip->priv;
 	uint8_t n_bytes = 0;
-	int8_t i, j;
+	int8_t i;
 
 	switch (bch->type) {

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

 	case ECC_BCH4:

drivers/mtd/nand/omap_gpmc.c static void omap_rotate_ecc_bch(struct mtd_info · line 391

Quoted 8 lines

drivers/mtd/nand/omap_gpmc.c +3 -1

@@ -338,7 +385,12 @@ static void omap_rotate_ecc_bch(struct mtd_info  
 *mtd, uint8_t *calc_ecc,
 		break;
 
 	case ECC_BCH16:
-		n_bytes = 28;
+		n_bytes = 26;
+
+		/* Last 2 register of ELM need to be zero */

s/register/registers/

drivers/mtd/nand/omap_gpmc.c static void omap_rotate_ecc_bch(struct mtd_info · line 405

Quoted 10 lines

drivers/mtd/nand/omap_gpmc.c +3 -2

@@ -347,16 +399,17 @@ static void omap_rotate_ecc_bch(struct mtd_info  
 *mtd, uint8_t *calc_ecc,
 		break;
 	}
 
-	for (i = 0, j = (n_bytes-1); i < n_bytes; i++, j--)
-		syndrome[i] =  calc_ecc[j];
+	for (i = 0; i < n_bytes; i++) {
+		syndrome[i] = calc_ecc[(n_bytes-1)-i];
+	}

Please put spaces around binary operators such as '-' (and again, no braces around single lines).

Quoted 13 lines

drivers/mtd/nand/omap_gpmc.c +3 -2

 void omap_nand_switch_ecc(int32_t hardware)
 {
+#ifndef CONFIG_AM33XX
 	struct nand_chip *nand;
 	struct mtd_info *mtd;
 
 	if (nand_curr_device < 0 ||
-	    nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE ||
-	    !nand_info[nand_curr_device].name) {
+		nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE ||
+		!nand_info[nand_curr_device].name) {
 		printf("Error: Can't switch ecc, no devices

available\n");

drivers/mtd/nand/omap_gpmc.c

Quoted 2 lines

drivers/mtd/nand/omap_gpmc.c +0 -0
 		return;
 	}

Please never have the continuation line of a conditional lined up with the body of the construct.

Quoted 8 lines

drivers/mtd/nand/omap_gpmc.c +0 -4

@@ -646,19 +702,6 @@ void omap_nand_switch_ecc(int32_t hardware)
 		nand->ecc.calculate = omap_calculate_ecc;
 		omap_hwecc_init(nand);
 		printf("HW ECC selected\n");
-#ifdef CONFIG_AM33XX
-	} else if (hardware == 2) {
-		nand->ecc.mode = NAND_ECC_HW;
-		nand->ecc.layout = &hw_bch8_nand_oob;

drivers/mtd/nand/omap_gpmc.c void omap_nand_switch_ecc(int32_t hardware) · line 705

Quoted 10 lines

drivers/mtd/nand/omap_gpmc.c +0 -9

-		nand->ecc.size = 512;
-		nand->ecc.bytes = 14;
-		nand->ecc.read_page = omap_read_page_bch;
-		nand->ecc.hwctl = omap_enable_ecc_bch;
-		nand->ecc.correct = omap_correct_data_bch;
-		nand->ecc.calculate = omap_calculate_ecc_bch;
-		omap_hwecc_init_bch(nand, NAND_ECC_READ);
-		printf("HW BCH8 selected\n");
-#endif
 	} else {

Explain.

include/linux/mtd/mtd-abi.h struct nand_oobfree { · line 131

Quoted 13 lines

include/linux/mtd/mtd-abi.h +1 -1

index 8bdd231..6979a2a 100644
@@ -125,7 +125,7 @@ struct nand_oobfree {
  */
 struct nand_ecclayout {
 	uint32_t eccbytes;
-	uint32_t eccpos[128];
+	uint32_t eccpos[208];
 	uint32_t oobavail;
 	struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES];
 };

Changes to generic code should ideally be separate patches.

-Scott

Tom Rini

2013-01-29 20:01 UTC #2 lore raw

PATCH 1/1 am335x: NAND: add BCH16 and 4k page size support

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 10 lines · Scott Wood

Quoted 9 lines · Jordy van Wolferen
This is tested with a custom AM3359 (rev 2.0) board. NAND chip:
MT29F16G08ABABAWP
This code allows me to boot from ROM code. The ROM code forces BCH16 on NAND chips with a 4k page size.
BCH16 is not enabled by default.

Quoted 6 lines

Quoted 1 line
---
Missing Signed-off-by (please read the "Sign your work" section of Documentation/SubmittingPatches in Linux and be sure that you meet the conditions of the Developer's Certificate of Origin before adding your sign off).

Note that this also appears to be versus a TI SDK tree and not mainline (but migrating the code shouldn't be overly difficult now that NAND support is mainline).

Quoted 2 lines

Could you explain the patch in a bit more detail? You say it is "not enabled by default" -- what would be required to enable it?

It needs to be auto-detected and switched to, just like how the ROM does (which is to say when the ROM picks up BCH16 and wants it, we do too).

[snip]

Quoted 9 lines

Quoted 7 lines
(quoted diff) +0 -0
b/include/linux/mtd/mtd-abi.h index 8bdd231..6979a2a 100644 --- a/include/linux/mtd/mtd-abi.h +++ b/include/linux/mtd/mtd-abi.h
(quoted diff) +0 -0
@@ -125,7 +125,7 @@ struct nand_oobfree { */ struct
nand_ecclayout { uint32_t eccbytes; - uint32_t eccpos[128]; +
uint32_t eccpos[208]; uint32_t oobavail; struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES]; };
Changes to generic code should ideally be separate patches.

True. And I wonder if the mtd re-sync covers this or not.

- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRCCquAAoJENk4IS6UOR1WcP4P+wUmTIMJlxEVP0JQseXL3fG1 gS0+KRYzFgceBkunfbgXcCGAqP2z3gcmyNEn0s/8dVkrN1F9MirUNz2VApqQwn0b lj8E/30e042EzM3sM/HyPO0S4yWBVf8GjYiRJMDb80gKOJelthrKNAjermIxqax6 dIHlSvV9pk6DYHTAmcgC7H3QLyHkwyZfSwii+ZRKm5hRPgpiQA2kI7klV8rEH8m7 x5d5c1Ry59hIrefmWWpJvrXhy9u8ipCzk37CCafyl3KbZJoOhZSHiXx73TebI1gP Sg1ONCvi8MkOZrMmC+iXqbKtrPYUGYApd7TonIoJg2yTlkIBevbTY4OLS/p81Btq HoJZ7PziCbeu4jZCVlS4Pz7OiIf5GyBbW1oJP/Uhz4tJAuSLJTrxHaiz7sUIZt3p zNr1ZrL7d5h6hlv3XBuERxRvAn2E4K+sSUJsS+C+8zi/rYMWfWxm5ncB4ygGqCNn QYvJSwWXK/yyFpq0NSbXZnHvBInwGcdrcQVJy3RftATxEW7B2EJKbLx8yJxcpoaU nDyN/NDYEeqZx3Vwf8q2YAv7S72vFP6Lwfy+bAbOeyc3emH6sYTE9iDt2EzTYuMM 3nDjgWZiNGDzJ7WzUv6ptDUIV9UQXxdwa7Ay4qc7aKbSx4t/LhjtIDXJYVh9Bbxq 3cYj8flqqYG9yQQn5xk/
=fOZt
-----END PGP SIGNATURE-----