Skip to contentSkip to conversation

keystone: nand: add additional nand ecclayout

PATCH Superseded Ivan Khoronzhuk 3 participants 14 messages May 16, 2014 – Jun 20, 2014

Subsystems (2)
Memory technology devices (MTD) NAND flash

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Ivan Khoronzhuk

author
#0 lore raw

cover keystone: nand: add additional nand ecclayout

This series adds a nand ecclayout command to davinci nand that allows the ecclayout of the current nand device to be changed during run time. This feature is useful when using u-boot to write something to nand flash that will be read by other applications, such as ROM bootloader, that expects a different ECC layout. In that case, change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Based on u-boot.git/master

WingMan Kwok (2):
common: cmd_nand: add nand ecclayout command
mtd: nand: davinci: allow to change ecclayout by ecclayout command

 common/cmd_nand.c               | 115 +++++++++++++++++++++++++++++++++++++++-
 drivers/mtd/nand/davinci_nand.c | 101 +++++++++++++++++++++++++++++++++++
 include/nand.h                  |  10 ++++
 3 files changed, 225 insertions(+), 1 deletion(-)
Signature · 1 line
1.8.3.2
author
#1 lore raw

PATCH 1/2 common: cmd_nand: add nand ecclayout command

From: WingMan Kwok <[email protected]>

This commit adds a nand ecclayout command that allows the ecclayout of the current nand device to be changed during run time. This feature is useful when using u-boot to write something to nand flash that will be read by other applications, such as ROM bootloader, that expects a different ECC layout. In that case, change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Signed-off-by WingMan Kwok <[email protected]>
Signed-off-by Ivan Khoronzhuk <[email protected]>
---
 common/cmd_nand.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/nand.h    |  10 +++++
 2 files changed, 124 insertions(+), 1 deletion(-)
common/cmd_nand.c +114 -1
index 04ab0f1..7cbe6fc 100644
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size, loff_t offset, int dev)
 	}
 }
 
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+static void nand_print_ecclayout_info(struct nand_ecclayout *p)
+{
+	int i;
+	struct nand_oobfree *oobfree;
+
+	if (!p)
+		return;
+
+	printf("  num ecc bytes: %d\n", p->eccbytes);
+	puts("  ecc pos:\n    ");
+
+	for (i = 0; i < p->eccbytes; i++) {
+		if (i && !(i % 9))
+			printf("\n    ");
+
+		printf("%2d ", p->eccpos[i]);
+	}
+
+	puts("\n  oobfree:\n");
+	puts("    offset length\n");
+
+	oobfree = p->oobfree;
+	for (i = 0; oobfree->length && i < MTD_MAX_OOBFREE_ENTRIES_LARGE; i++) {
+		printf("     %2d    %2d\n", oobfree->offset, oobfree->length);
+		oobfree++;
+	}
+}
+
+static void nand_print_device_ecclayout(int dev)
+{
+	int idx;
+	nand_info_t *nand = &nand_info[dev];
+	struct nand_chip *chip = nand->priv;
+
+	idx = board_nand_ecclayout_get_idx(chip, chip->ecc.layout);
+
+	if (idx < 0) {
+		puts("no ecc layout\n");
+		return;
+	}
+
+	printf("\necc layout %d:\n", idx);
+	nand_print_ecclayout_info(chip->ecc.layout);
+}
+#endif
+
 static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i, ret = 0;

@@ -506,8 +553,12 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			putc('\n');
 			if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE)
 				puts("no devices available\n");
-			else
+			else {
 				nand_print_and_set_info(dev);
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+				nand_print_device_ecclayout(dev);
+#endif
+			}
 			return 0;
 		}
 

@@ -836,6 +887,60 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 #endif
 
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+	if (strcmp(cmd, "ecclayout") == 0) {
+		int idx;
+		struct nand_ecclayout *p;
+		nand_info_t *nand = &nand_info[dev];
+		struct nand_chip *chip = nand->priv;
+
+		if (argc < 3) {
+			puts("Current device ecclayout:\n");
+			nand_print_device_ecclayout(dev);
+			return 0;
+		}
+
+		if (!strcmp(argv[2], "set")) {
+			if (argc < 4)
+				return 1;
+
+			idx = (int)simple_strtoul(argv[3], NULL, 10);
+			if (!board_nand_ecclayout_set(chip, idx)) {
+				p = chip->ecc.layout;
+				p->oobavail = 0;
+				for (i = 0; p->oobfree[i].length &&
+						i < ARRAY_SIZE(p->oobfree); i++)
+					p->oobavail += p->oobfree[i].length;
+
+				nand->oobavail = p->oobavail;
+			} else {
+				printf("Setting current device"
+				       " to ecc layout %d FAILED!\n", idx);
+			}
+
+			return 0;
+		}
+
+		if (strcmp(argv[2], "all") != 0)
+			return 1;
+
+		/* show all available ecc layouts */
+		puts("Available ecc layouts:\n");
+
+		idx = 0;
+		while (1) {
+			p = board_nand_ecclayout_get_layout(chip, idx);
+			if (!p)
+				break;
+
+			printf("\n ecc layout %d:\n", idx++);
+			nand_print_ecclayout_info(p);
+		}
+
+		return 0;
+	}
+#endif
+
 usage:
 	return CMD_RET_USAGE;
 }

@@ -890,6 +995,14 @@ static char nand_help_text[] =
 	"nand env.oob set off|partition - set enviromnent offset\n"
 	"nand env.oob get - get environment offset"
 #endif
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+	"\n"
+	"nand ecclayout <set <idx>|all> - show or set ecclayout"
+	" of current device\n"
+	"    'all' shows all available ecc layouts for setting"
+	" to current device\n"
+	"    'set' sets current device ecc layout to layout indexed by idx\n"
+#endif
 	"";
 #endif
include/nand.h +10 -0
index fc735d1..0e8a093 100644
@@ -155,6 +155,16 @@ void nand_deselect(void);
 void board_nand_select_device(struct nand_chip *nand, int chip);
 #endif
 
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+int board_nand_ecclayout_get_idx(struct nand_chip *nand,
+				 struct nand_ecclayout *p);
+
+struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
+						       int idx);
+
+int board_nand_ecclayout_set(struct nand_chip *nand, int idx);
+#endif
+
 __attribute__((noreturn)) void nand_boot(void);
 
 #endif
Signature · 1 line
1.8.3.2
#2 lore raw

PATCH 1/2 common: cmd_nand: add nand ecclayout command

Quoted 34 lines · Ivan

From: WingMan Kwok <[email protected]>

This commit adds a nand ecclayout command that allows the ecclayout of the current nand device to be changed during run time. This feature is useful when using u-boot to write something to nand flash that will be read by other applications, such as ROM bootloader, that expects a different ECC layout. In that case, change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Signed-off-by WingMan Kwok <[email protected]>
Signed-off-by Ivan Khoronzhuk <[email protected]>

---
common/cmd_nand.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++-

 include/nand.h    |  10 +++++
 2 files changed, 124 insertions(+), 1 deletion(-)
common/cmd_nand.c +8 -0
index 04ab0f1..7cbe6fc 100644
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size, loff_t offset, int dev)
 	}
 }
 
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+static void nand_print_ecclayout_info(struct nand_ecclayout *p)
+{
+	int i;
+	struct nand_oobfree *oobfree;
+
+	if (!p)
+		return;

common/cmd_nand.c static void adjust_size_for_badblocks(loff_t *size, loff_t offset, int dev) · line 482

Quoted 10 lines
common/cmd_nand.c +10 -0
+
+	printf("  num ecc bytes: %d\n", p->eccbytes);
+	puts("  ecc pos:\n    ");
+
+	for (i = 0; i < p->eccbytes; i++) {
+		if (i && !(i % 9))
+			printf("\n    ");
+
+		printf("%2d ", p->eccpos[i]);
+	}

Why 9?

Quoted 34 lines
common/cmd_nand.c +28 -0
+	puts("\n  oobfree:\n");
+	puts("    offset length\n");
+
+	oobfree = p->oobfree;
+	for (i = 0; oobfree->length && i < MTD_MAX_OOBFREE_ENTRIES_LARGE; i++) {
+		printf("     %2d    %2d\n", oobfree->offset, oobfree->length);
+		oobfree++;
+	}
+}
+
+static void nand_print_device_ecclayout(int dev)
+{
+	int idx;
+	nand_info_t *nand = &nand_info[dev];
+	struct nand_chip *chip = nand->priv;
+
+	idx = board_nand_ecclayout_get_idx(chip, chip->ecc.layout);
+
+	if (idx < 0) {
+		puts("no ecc layout\n");
+		return;
+	}
+
+	printf("\necc layout %d:\n", idx);
+	nand_print_ecclayout_info(chip->ecc.layout);
+}
+#endif
+
 static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i, ret = 0;

@@ -506,8 +553,12 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			putc('\n');
 			if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE)

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 563

Quoted 10 lines
common/cmd_nand.c +5 -1
 				puts("no devices available\n");
-			else
+			else {
 				nand_print_and_set_info(dev);
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+				nand_print_device_ecclayout(dev);
+#endif
+			}
 			return 0;
 		}

Braces are needed on both sides of if/else if used on one side.

It would be better to provide an inline no-op stub when CONFIG_CMD_NAND_ECCLAYOUT, than ifdeffing at the call site.

Does it really make sense to dump this information here, when the user could use the ecclayout command instead?

Quoted 20 lines
common/cmd_nand.c +16 -0
@@ -836,6 +887,60 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 #endif
 
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+	if (strcmp(cmd, "ecclayout") == 0) {
+		int idx;
+		struct nand_ecclayout *p;
+		nand_info_t *nand = &nand_info[dev];
+		struct nand_chip *chip = nand->priv;
+
+		if (argc < 3) {
+			puts("Current device ecclayout:\n");
+			nand_print_device_ecclayout(dev);
+			return 0;
+		}
+
+		if (!strcmp(argv[2], "set")) {
+			if (argc < 4)
+				return 1;

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 915

Quoted 10 lines
common/cmd_nand.c +10 -0
+
+			idx = (int)simple_strtoul(argv[3], NULL, 10);
+			if (!board_nand_ecclayout_set(chip, idx)) {
+				p = chip->ecc.layout;
+				p->oobavail = 0;
+				for (i = 0; p->oobfree[i].length &&
+						i < ARRAY_SIZE(p->oobfree); i++)
+					p->oobavail += p->oobfree[i].length;
+
+				nand->oobavail = p->oobavail;

Shouldn't board_nand_ecclayout_set() take care of this? Possibly via common helper functions, but not here.

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 918

Quoted 3 lines
common/cmd_nand.c +3 -0
+			} else {
+				printf("Setting current device"
+				       " to ecc layout %d FAILED!\n", idx);

Don't wrap user-visible strings (checkpatch should have warned you about this).

Quoted 10 lines
common/cmd_nand.c +10 -0
+			}
+
+			return 0;
+		}
+
+		if (strcmp(argv[2], "all") != 0)
+			return 1;
+
+		/* show all available ecc layouts */
+		puts("Available ecc layouts:\n");

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 938

Quoted 10 lines
common/cmd_nand.c +10 -0
+
+		idx = 0;
+		while (1) {
+			p = board_nand_ecclayout_get_layout(chip, idx);
+			if (!p)
+				break;
+
+			printf("\n ecc layout %d:\n", idx++);
+			nand_print_ecclayout_info(p);
+		}

Please use a for loop rather than hiding the increment in the loop body.

Quoted 10 lines
common/cmd_nand.c +5 -0
+
+		return 0;
+	}
+#endif
+
 usage:
 	return CMD_RET_USAGE;
 }

@@ -890,6 +995,14 @@ static char nand_help_text[] =
 	"nand env.oob set off|partition - set enviromnent offset\n"

common/cmd_nand.c static char nand_help_text[] = · line 1005

Quoted 10 lines
common/cmd_nand.c +8 -0
 	"nand env.oob get - get environment offset"
 #endif
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+	"\n"
+	"nand ecclayout <set <idx>|all> - show or set ecclayout"
+	" of current device\n"
+	"    'all' shows all available ecc layouts for setting"
+	" to current device\n"
+	"    'set' sets current device ecc layout to layout indexed by idx\n"
+#endif

This help text doesn't include the plain "nand ecclayout" to show the current layout.

Quoted 7 lines
include/nand.h +0 -0
index fc735d1..0e8a093 100644
@@ -155,6 +155,16 @@ void nand_deselect(void);
 void board_nand_select_device(struct nand_chip *nand, int chip);
 #endif

include/nand.h void nand_deselect(void); · line 166

Quoted 10 lines
include/nand.h +9 -0
 
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+int board_nand_ecclayout_get_idx(struct nand_chip *nand,
+				 struct nand_ecclayout *p);
+
+struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
+						       int idx);
+
+int board_nand_ecclayout_set(struct nand_chip *nand, int idx);
+#endif

Don't ifdef prototypes.

-Scott

author
#3 lore raw

PATCH 1/2 common: cmd_nand: add nand ecclayout command

Quoted 38 lines · Scott Wood
Quoted 37 lines · Ivan

From: WingMan Kwok <[email protected]>

This commit adds a nand ecclayout command that allows the ecclayout of the current nand device to be changed during run time. This feature is useful when using u-boot to write something to nand flash that will be read by other applications, such as ROM bootloader, that expects a different ECC layout. In that case, change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Signed-off-by WingMan Kwok <[email protected]>
Signed-off-by Ivan Khoronzhuk <[email protected]>

---
common/cmd_nand.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++-

  include/nand.h    |  10 +++++
  2 files changed, 124 insertions(+), 1 deletion(-)
common/cmd_nand.c +11 -0
index 04ab0f1..7cbe6fc 100644
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size, loff_t offset, int dev)
  	}
  }
 
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+static void nand_print_ecclayout_info(struct nand_ecclayout *p)
+{
+	int i;
+	struct nand_oobfree *oobfree;
+
+	if (!p)
+		return;
+
+	printf("  num ecc bytes: %d\n", p->eccbytes);
+	puts("  ecc pos:\n    ");
Quoted 8 lines

common/cmd_nand.c static void adjust_size_for_badblocks(loff_t *size, loff_t offset, int dev) · line 482

Quoted 7 lines
common/cmd_nand.c +7 -0
+
+	for (i = 0; i < p->eccbytes; i++) {
+		if (i && !(i % 9))
+			printf("\n    ");
+
+		printf("%2d ", p->eccpos[i]);
+	}

Why 9?

It's to print a new line on every 9th character position. I'll add a comment.

Quoted 38 lines
Quoted 38 lines
common/cmd_nand.c +29 -1
+	puts("\n  oobfree:\n");
+	puts("    offset length\n");
+
+	oobfree = p->oobfree;
+	for (i = 0; oobfree->length && i < MTD_MAX_OOBFREE_ENTRIES_LARGE; i++) {
+		printf("     %2d    %2d\n", oobfree->offset, oobfree->length);
+		oobfree++;
+	}
+}
+
+static void nand_print_device_ecclayout(int dev)
+{
+	int idx;
+	nand_info_t *nand = &nand_info[dev];
+	struct nand_chip *chip = nand->priv;
+
+	idx = board_nand_ecclayout_get_idx(chip, chip->ecc.layout);
+
+	if (idx < 0) {
+		puts("no ecc layout\n");
+		return;
+	}
+
+	printf("\necc layout %d:\n", idx);
+	nand_print_ecclayout_info(chip->ecc.layout);
+}
+#endif
+
  static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  {
  	int i, ret = 0;

@@ -506,8 +553,12 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  			putc('\n');
  			if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE)
  				puts("no devices available\n");
-			else
+			else {
Quoted 8 lines

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 563

Quoted 7 lines
common/cmd_nand.c +4 -0
  				nand_print_and_set_info(dev);
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+				nand_print_device_ecclayout(dev);
+#endif
+			}
  			return 0;
  		}

Braces are needed on both sides of if/else if used on one side.

Yes.

Quoted 3 lines

It would be better to provide an inline no-op stub when CONFIG_CMD_NAND_ECCLAYOUT, than ifdeffing at the call site.

Yes. It's better.

Quoted 3 lines

Does it really make sense to dump this information here, when the user could use the ecclayout command instead?

Yes. Seems it's redundant. I'll better delete it.

Quoted 25 lines
Quoted 25 lines
common/cmd_nand.c +20 -0
@@ -836,6 +887,60 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  	}
  #endif
 
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+	if (strcmp(cmd, "ecclayout") == 0) {
+		int idx;
+		struct nand_ecclayout *p;
+		nand_info_t *nand = &nand_info[dev];
+		struct nand_chip *chip = nand->priv;
+
+		if (argc < 3) {
+			puts("Current device ecclayout:\n");
+			nand_print_device_ecclayout(dev);
+			return 0;
+		}
+
+		if (!strcmp(argv[2], "set")) {
+			if (argc < 4)
+				return 1;
+
+			idx = (int)simple_strtoul(argv[3], NULL, 10);
+			if (!board_nand_ecclayout_set(chip, idx)) {
+				p = chip->ecc.layout;
Quoted 8 lines

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 915

Quoted 6 lines
common/cmd_nand.c +6 -0
+				p->oobavail = 0;
+				for (i = 0; p->oobfree[i].length &&
+						i < ARRAY_SIZE(p->oobfree); i++)
+					p->oobavail += p->oobfree[i].length;
+
+				nand->oobavail = p->oobavail;

Shouldn't board_nand_ecclayout_set() take care of this? Possibly via common helper functions, but not here.

Yes, It should be moved to board_nand_ecclayout_set()

Quoted 5 lines

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 918

Quoted 3 lines
common/cmd_nand.c +3 -0
+			} else {
+				printf("Setting current device"
+				       " to ecc layout %d FAILED!\n", idx);

Don't wrap user-visible strings (checkpatch should have warned you about this).

For user's eyes it's not wrapped.

Quoted 13 lines
Quoted 13 lines
common/cmd_nand.c +13 -0
+			}
+
+			return 0;
+		}
+
+		if (strcmp(argv[2], "all") != 0)
+			return 1;
+
+		/* show all available ecc layouts */
+		puts("Available ecc layouts:\n");
+
+		idx = 0;
+		while (1) {
Quoted 8 lines

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 938

Quoted 7 lines
common/cmd_nand.c +7 -0
+			p = board_nand_ecclayout_get_layout(chip, idx);
+			if (!p)
+				break;
+
+			printf("\n ecc layout %d:\n", idx++);
+			nand_print_ecclayout_info(p);
+		}

Please use a for loop rather than hiding the increment in the loop body.

Ok.

Quoted 15 lines
Quoted 15 lines
common/cmd_nand.c +7 -0
+
+		return 0;
+	}
+#endif
+
  usage:
  	return CMD_RET_USAGE;
  }

@@ -890,6 +995,14 @@ static char nand_help_text[] =
  	"nand env.oob set off|partition - set enviromnent offset\n"
  	"nand env.oob get - get environment offset"
  #endif
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+	"\n"
Quoted 8 lines

common/cmd_nand.c static char nand_help_text[] = · line 1005

Quoted 6 lines
common/cmd_nand.c +6 -0
+	"nand ecclayout <set <idx>|all> - show or set ecclayout"
+	" of current device\n"
+	"    'all' shows all available ecc layouts for setting"
+	" to current device\n"
+	"    'set' sets current device ecc layout to layout indexed by idx\n"
+#endif

This help text doesn't include the plain "nand ecclayout" to show the current layout.

Yes.

Quoted 8 lines
Quoted 8 lines
include/nand.h +0 -0
index fc735d1..0e8a093 100644
@@ -155,6 +155,16 @@ void nand_deselect(void);
  void board_nand_select_device(struct nand_chip *nand, int chip);
  #endif
Quoted 10 lines

include/nand.h void nand_deselect(void); · line 166

Quoted 9 lines
include/nand.h +9 -0
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+int board_nand_ecclayout_get_idx(struct nand_chip *nand,
+				 struct nand_ecclayout *p);
+
+struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
+						       int idx);
+
+int board_nand_ecclayout_set(struct nand_chip *nand, int idx);
+#endif

Don't ifdef prototypes.

Ok. Thanks! I will repost soon.

Quoted 2 lines

-Scott

Signature · 2 lines
Regards,
Ivan Khoronzhuk
#4 lore raw

PATCH 1/2 common: cmd_nand: add nand ecclayout command

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
common/cmd_nand.c +6 -0
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size,
 
+       for (i = 0; i < p->eccbytes; i++) {
+               if (i && !(i % 9))
+                       printf("\n    ");
+
+               printf("%2d ", p->eccpos[i]);
+       }
Quoted 5 lines
Quoted 1 line

Why 9?

It's to print a new line on every 9th character position. I'll add a comment.

OK, Scott, breath... I got this one. It'll be OK...

Ivan,
I am confident Scott understood that a newline would be generated every ninth-character. We all get that. I think what Scott was asking was why the value 9 was chosen? Why not 10? Or 8? Or 145? Was it to fit some arbitrary line length or screen size? Would it make more sense to use something familiar like a base 10 or half of base-16?

Thanks,
jdl

#5 lore raw

PATCH 1/2 common: cmd_nand: add nand ecclayout command

Quoted 19 lines · Jon Loeliger
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
common/cmd_nand.c +6 -0
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size,
 
+       for (i = 0; i < p->eccbytes; i++) {
+               if (i && !(i % 9))
+                       printf("\n    ");
+
+               printf("%2d ", p->eccpos[i]);
+       }
Quoted 5 lines
Quoted 1 line

Why 9?

It's to print a new line on every 9th character position. I'll add a comment.

OK, Scott, breath... I got this one. It'll be OK...

Quoted 7 lines

Ivan,
I am confident Scott understood that a newline would be generated every ninth-character. We all get that. I think what Scott was asking was why the value 9 was chosen? Why not 10? Or 8? Or 145? Was it to fit some arbitrary line length or screen size? Would it make more sense to use something familiar like a base 10 or half of base-16?

More specifically, it neither avoids a division (as a power of two would) nor does it seem to match the ecc size of the layouts used by the davinci driver (which is the only user of this so far), nor is it anywhere near 80 columns.

Also, why is the field width two characters, when ecc positions can exceed 100?

-Scott

author
#6 lore raw

PATCH 1/2 common: cmd_nand: add nand ecclayout command

Quoted 28 lines · Scott Wood
Quoted 15 lines · Jon Loeliger
Quoted 11 lines
Quoted 10 lines

common/cmd_nand.c static void adjust_size_for_badblocks(loff_t *size, · line 467

Quoted 9 lines
common/cmd_nand.c +6 -0
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size,
+       for (i = 0; i < p->eccbytes; i++) {
+               if (i && !(i % 9))
+                       printf("\n    ");
+
+               printf("%2d ", p->eccpos[i]);
+       }

Why 9?

Quoted 2 lines

It's to print a new line on every 9th character position. I'll add a comment.

OK, Scott, breath... I got this one. It'll be OK...

Quoted 7 lines

Ivan,
I am confident Scott understood that a newline would be generated every ninth-character. We all get that. I think what Scott was asking was why the value 9 was chosen? Why not 10? Or 8? Or 145? Was it to fit some arbitrary line length or screen size? Would it make more sense to use something familiar like a base 10 or half of base-16?

More specifically, it neither avoids a division (as a power of two would) nor does it seem to match the ecc size of the layouts used by the davinci driver (which is the only user of this so far), nor is it anywhere near 80 columns.

Quoted 6 lines

Also, why is the field width two characters, when ecc positions can exceed 100?

-Scott

Ok, I'll try to correct as you proposed. I dislike it also ... Thanks.

Signature · 2 lines
Regards,
Ivan Khoronzhuk
#7 lore raw

PATCH 1/2 common: cmd_nand: add nand ecclayout command

In reply to Ivan Khoronzhuk
Quoted 9 lines · Ivan Khoronzhuk
Quoted 6 lines · Scott Wood

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 918

Quoted 3 lines · Ivan
common/cmd_nand.c +3 -0
+			} else {
+				printf("Setting current device"
+				       " to ecc layout %d FAILED!\n", idx);

Don't wrap user-visible strings (checkpatch should have warned you about this).

For user's eyes it's not wrapped.

That's not the point. Wrapping it in the source means the user can't grep for the string. Checkpatch should have issued a warning about this. This is an exception to the 80 column rule.

-Scott

author
#8 lore raw

PATCH 1/2 common: cmd_nand: add nand ecclayout command

Quoted 8 lines · Scott Wood
Quoted 7 lines · Ivan Khoronzhuk
Quoted 6 lines · Scott Wood

common/cmd_nand.c static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 918

Quoted 3 lines · Ivan
common/cmd_nand.c +3 -0
+			} else {
+				printf("Setting current device"
+				       " to ecc layout %d FAILED!\n", idx);

Don't wrap user-visible strings (checkpatch should have warned you about this).

Quoted 8 lines
Quoted 1 line

For user's eyes it's not wrapped.

That's not the point. Wrapping it in the source means the user can't grep for the string. Checkpatch should have issued a warning about this. This is an exception to the 80 column rule.

-Scott

Ok.

Signature · 2 lines
Regards,
Ivan Khoronzhuk
author
#9 lore raw

PATCH 2/2 mtd: nand: davinci: allow to change ecclayout by ecclayout command

In reply to Ivan Khoronzhuk

From: WingMan Kwok <[email protected]>

This patch adds opportunity to change ecclayout of current nand device during runtime. So we can change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Signed-off-by Hao Zhang <[email protected]>
Signed-off-by WingMan Kwok <[email protected]>
Signed-off-by Ivan Khoronzhuk <[email protected]>
---
 drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)
drivers/mtd/nand/davinci_nand.c +101 -0
index 75b03a7..b33de0d 100644
@@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = {
 #endif
 };
 
+#if defined(CONFIG_CMD_NAND_ECCLAYOUT)
+#if defined(CONFIG_SYS_NAND_PAGE_2K)
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 40,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+	},
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+	},
+};
+#elif defined(CONFIG_SYS_NAND_PAGE_4K)
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 80,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+		70, 71,
+		72, 73, 74, 75, 76, 77, 78, 79,
+		86, 87,
+		88, 89, 90, 91, 92, 93, 94, 95,
+		102, 103,
+		104, 105, 106, 107, 108, 109, 110, 111,
+		118, 119,
+		120, 121, 122, 123, 124, 125, 126, 127,
+		},
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+		{.offset = 64, .length = 6, },
+		{.offset = 80, .length = 6, },
+		{.offset = 96, .length = 6, },
+		{.offset = 112, .length = 6, },
+	},
+};
+#endif
+
+#define NAND_ECCLAYOUT_NUM 2
+
+struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = {
+	&nand_davinci_4bit_layout_oobfirst,
+	&nand_keystone_rbl_4bit_layout_oobfirst,
+};
+
+int board_nand_ecclayout_get_idx(struct nand_chip *nand,
+				 struct nand_ecclayout *p)
+{
+	int i;
+
+	if (!p)
+		return -1;
+
+	for (i = 0; i < NAND_ECCLAYOUT_NUM; i++)
+		if (davinci_nand_ecclayouts[i] == p)
+			return i;
+
+	return -1;
+}
+
+struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
+						       int idx)
+{
+	if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM))
+		return davinci_nand_ecclayouts[idx];
+	else
+		return NULL;
+}
+
+int board_nand_ecclayout_set(struct nand_chip *nand, int idx)
+{
+	if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM)
+		return -1;
+
+	nand->ecc.layout = davinci_nand_ecclayouts[idx];
+
+	return 0;
+}
+#endif
+
 static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode)
 {
 	u32 val;

@@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand)
 	nand->ecc.calculate = nand_davinci_4bit_calculate_ecc;
 	nand->ecc.correct = nand_davinci_4bit_correct_data;
 	nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc;
+#ifndef CONFIG_CMD_NAND_ECCLAYOUT
 	nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst;
+#else
+	nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst;
+#endif
 #endif
 	/* Set address of hardware control function */
 	nand->cmd_ctrl = nand_davinci_hwcontrol;
Signature · 1 line
1.8.3.2
#10 lore raw

PATCH 2/2 mtd: nand: davinci: allow to change ecclayout by ecclayout command

Quoted 20 lines · Ivan

From: WingMan Kwok <[email protected]>

This patch adds opportunity to change ecclayout of current nand device during runtime. So we can change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Signed-off-by Hao Zhang <[email protected]>
Signed-off-by WingMan Kwok <[email protected]>
Signed-off-by Ivan Khoronzhuk <[email protected]>

---
drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++

 1 file changed, 101 insertions(+)
drivers/mtd/nand/davinci_nand.c +0 -0
index 75b03a7..b33de0d 100644

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 310

Quoted 5 lines
drivers/mtd/nand/davinci_nand.c +2 -0
@@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = {
 #endif
 };
 
+#if defined(CONFIG_CMD_NAND_ECCLAYOUT)
+#if defined(CONFIG_SYS_NAND_PAGE_2K)

Undocumented CONFIG symbol (yes, I know it's not new)... Also redundant with CONFIG_SYS_NAND_PAGE_SIZE (though that's only documented in the context of SPL).

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 322

Quoted 12 lines
drivers/mtd/nand/davinci_nand.c +12 -0
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 40,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+	},

Why the odd 2/8 pattern?

Quoted 52 lines
drivers/mtd/nand/davinci_nand.c +52 -0
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+	},
+};
+#elif defined(CONFIG_SYS_NAND_PAGE_4K)
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 80,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+		70, 71,
+		72, 73, 74, 75, 76, 77, 78, 79,
+		86, 87,
+		88, 89, 90, 91, 92, 93, 94, 95,
+		102, 103,
+		104, 105, 106, 107, 108, 109, 110, 111,
+		118, 119,
+		120, 121, 122, 123, 124, 125, 126, 127,
+		},
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+		{.offset = 64, .length = 6, },
+		{.offset = 80, .length = 6, },
+		{.offset = 96, .length = 6, },
+		{.offset = 112, .length = 6, },
+	},
+};
+#endif
+
+#define NAND_ECCLAYOUT_NUM 2
+
+struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = {
+	&nand_davinci_4bit_layout_oobfirst,
+	&nand_keystone_rbl_4bit_layout_oobfirst,
+};
+
+int board_nand_ecclayout_get_idx(struct nand_chip *nand,
+				 struct nand_ecclayout *p)
+{
+	int i;

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 384

Quoted 10 lines
drivers/mtd/nand/davinci_nand.c +10 -0
+
+	if (!p)
+		return -1;
+
+	for (i = 0; i < NAND_ECCLAYOUT_NUM; i++)
+		if (davinci_nand_ecclayouts[i] == p)
+			return i;
+
+	return -1;
+}

Use ARRAY_SIZE().

Quoted 10 lines
drivers/mtd/nand/davinci_nand.c +10 -0
+
+struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
+						       int idx)
+{
+	if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM))
+		return davinci_nand_ecclayouts[idx];
+	else
+		return NULL;
+}
+

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 404

Quoted 10 lines
drivers/mtd/nand/davinci_nand.c +10 -0
+int board_nand_ecclayout_set(struct nand_chip *nand, int idx)
+{
+	if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM)
+		return -1;
+
+	nand->ecc.layout = davinci_nand_ecclayouts[idx];
+
+	return 0;
+}
+#endif

Put a comment on this indicating what condition the #endif is ending.

drivers/mtd/nand/davinci_nand.c void davinci_nand_init(struct nand_chip *nand) · line 735

Quoted 12 lines
drivers/mtd/nand/davinci_nand.c +4 -0
 static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode)
 {
 	u32 val;

@@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand)
 	nand->ecc.calculate = nand_davinci_4bit_calculate_ecc;
 	nand->ecc.correct = nand_davinci_4bit_correct_data;
 	nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc;
+#ifndef CONFIG_CMD_NAND_ECCLAYOUT
 	nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst;
+#else
+	nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst;
+#endif

Use "ifdef/else" rather than "ifndef/else".

Why does enabling layout selection change the default layout? Nothing in the changelog suggests that.

-Scott

author
#11 lore raw

PATCH 2/2 mtd: nand: davinci: allow to change ecclayout by ecclayout command

Quoted 30 lines · Scott Wood
Quoted 20 lines · Ivan

From: WingMan Kwok <[email protected]>

This patch adds opportunity to change ecclayout of current nand device during runtime. So we can change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Signed-off-by Hao Zhang <[email protected]>
Signed-off-by WingMan Kwok <[email protected]>
Signed-off-by Ivan Khoronzhuk <[email protected]>

---
drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++

  1 file changed, 101 insertions(+)
drivers/mtd/nand/davinci_nand.c +0 -0
index 75b03a7..b33de0d 100644

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 310

Quoted 5 lines
drivers/mtd/nand/davinci_nand.c +2 -0
@@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = {
  #endif
  };
 
+#if defined(CONFIG_CMD_NAND_ECCLAYOUT)
+#if defined(CONFIG_SYS_NAND_PAGE_2K)

Undocumented CONFIG symbol (yes, I know it's not new)... Also redundant with CONFIG_SYS_NAND_PAGE_SIZE (though that's only documented in the context of SPL).

Quoted 13 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 322

Quoted 12 lines
drivers/mtd/nand/davinci_nand.c +12 -0
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 40,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+	},

Why the odd 2/8 pattern?

...I'll align.

Quoted 56 lines
Quoted 56 lines
drivers/mtd/nand/davinci_nand.c +55 -0
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+	},
+};
+#elif defined(CONFIG_SYS_NAND_PAGE_4K)
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 80,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+		70, 71,
+		72, 73, 74, 75, 76, 77, 78, 79,
+		86, 87,
+		88, 89, 90, 91, 92, 93, 94, 95,
+		102, 103,
+		104, 105, 106, 107, 108, 109, 110, 111,
+		118, 119,
+		120, 121, 122, 123, 124, 125, 126, 127,
+		},
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+		{.offset = 64, .length = 6, },
+		{.offset = 80, .length = 6, },
+		{.offset = 96, .length = 6, },
+		{.offset = 112, .length = 6, },
+	},
+};
+#endif
+
+#define NAND_ECCLAYOUT_NUM 2
+
+struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = {
+	&nand_davinci_4bit_layout_oobfirst,
+	&nand_keystone_rbl_4bit_layout_oobfirst,
+};
+
+int board_nand_ecclayout_get_idx(struct nand_chip *nand,
+				 struct nand_ecclayout *p)
+{
+	int i;
+
+	if (!p)
+		return -1;
Quoted 8 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 384

Quoted 7 lines
drivers/mtd/nand/davinci_nand.c +7 -0
+
+	for (i = 0; i < NAND_ECCLAYOUT_NUM; i++)
+		if (davinci_nand_ecclayouts[i] == p)
+			return i;
+
+	return -1;
+}

Use ARRAY_SIZE().

Ok.

Quoted 14 lines
Quoted 14 lines
drivers/mtd/nand/davinci_nand.c +13 -0
+
+struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
+						       int idx)
+{
+	if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM))
+		return davinci_nand_ecclayouts[idx];
+	else
+		return NULL;
+}
+
+int board_nand_ecclayout_set(struct nand_chip *nand, int idx)
+{
+	if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM)
Quoted 8 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 404

Quoted 7 lines
drivers/mtd/nand/davinci_nand.c +7 -0
+		return -1;
+
+	nand->ecc.layout = davinci_nand_ecclayouts[idx];
+
+	return 0;
+}
+#endif

Put a comment on this indicating what condition the #endif is ending.

Ok.

Quoted 15 lines

drivers/mtd/nand/davinci_nand.c void davinci_nand_init(struct nand_chip *nand) · line 735

Quoted 13 lines
drivers/mtd/nand/davinci_nand.c +4 -0
  static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode)
  {
  	u32 val;

@@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand)
  	nand->ecc.calculate = nand_davinci_4bit_calculate_ecc;
  	nand->ecc.correct = nand_davinci_4bit_correct_data;
  	nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc;
+#ifndef CONFIG_CMD_NAND_ECCLAYOUT
  	nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst;
+#else
+	nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst;
+#endif

Use "ifdef/else" rather than "ifndef/else".

Quoted 2 lines

Why does enabling layout selection change the default layout? Nothing in the changelog suggests that.

It's not correct. I will move it to board code.
I'll better do smth like the following:

In board config:
#define CONFIG_SYS_NAND_SELF_INIT in board config:

In nand davinci header:
#define NAND_DAVINCI_4BIT_LAYOUT 0
#define NAND_KEYSTONE_RBL_4BIT_LAYOUT 1

In board file:
int board_nand_init(struct nand_chip *chip)

{
     davinci_nand_init(chip);
     chip->ecc.layout =

board_nand_ecclayout_get_layout(NAND_KEYSTONE_RBL_4BIT_LAYOUT);

     return 0;
}
Quoted 2 lines

-Scott

Thanks a lot Scott, I will send the new series soon.

Signature · 2 lines
Regards,
Ivan Khoronzhuk
#12 lore raw

PATCH 2/2 mtd: nand: davinci: allow to change ecclayout by ecclayout command

Quoted 169 lines · Ivan Khoronzhuk
Quoted 30 lines · Scott Wood
Quoted 20 lines · Ivan

From: WingMan Kwok <[email protected]>

This patch adds opportunity to change ecclayout of current nand device during runtime. So we can change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Signed-off-by Hao Zhang <[email protected]>
Signed-off-by WingMan Kwok <[email protected]>
Signed-off-by Ivan Khoronzhuk <[email protected]>

---
drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++

  1 file changed, 101 insertions(+)
drivers/mtd/nand/davinci_nand.c +0 -0
index 75b03a7..b33de0d 100644

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 310

Quoted 5 lines
drivers/mtd/nand/davinci_nand.c +2 -0
@@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = {
  #endif
  };
 
+#if defined(CONFIG_CMD_NAND_ECCLAYOUT)
+#if defined(CONFIG_SYS_NAND_PAGE_2K)

Undocumented CONFIG symbol (yes, I know it's not new)... Also redundant with CONFIG_SYS_NAND_PAGE_SIZE (though that's only documented in the context of SPL).

Quoted 13 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 322

Quoted 12 lines
drivers/mtd/nand/davinci_nand.c +12 -0
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 40,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+	},

Why the odd 2/8 pattern?

...I'll align.

Quoted 56 lines
Quoted 56 lines
drivers/mtd/nand/davinci_nand.c +55 -0
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+	},
+};
+#elif defined(CONFIG_SYS_NAND_PAGE_4K)
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 80,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+		70, 71,
+		72, 73, 74, 75, 76, 77, 78, 79,
+		86, 87,
+		88, 89, 90, 91, 92, 93, 94, 95,
+		102, 103,
+		104, 105, 106, 107, 108, 109, 110, 111,
+		118, 119,
+		120, 121, 122, 123, 124, 125, 126, 127,
+		},
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+		{.offset = 64, .length = 6, },
+		{.offset = 80, .length = 6, },
+		{.offset = 96, .length = 6, },
+		{.offset = 112, .length = 6, },
+	},
+};
+#endif
+
+#define NAND_ECCLAYOUT_NUM 2
+
+struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = {
+	&nand_davinci_4bit_layout_oobfirst,
+	&nand_keystone_rbl_4bit_layout_oobfirst,
+};
+
+int board_nand_ecclayout_get_idx(struct nand_chip *nand,
+				 struct nand_ecclayout *p)
+{
+	int i;
+
+	if (!p)
+		return -1;
Quoted 8 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 384

Quoted 7 lines
drivers/mtd/nand/davinci_nand.c +7 -0
+
+	for (i = 0; i < NAND_ECCLAYOUT_NUM; i++)
+		if (davinci_nand_ecclayouts[i] == p)
+			return i;
+
+	return -1;
+}

Use ARRAY_SIZE().

Ok.

Quoted 14 lines
Quoted 14 lines
drivers/mtd/nand/davinci_nand.c +13 -0
+
+struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
+						       int idx)
+{
+	if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM))
+		return davinci_nand_ecclayouts[idx];
+	else
+		return NULL;
+}
+
+int board_nand_ecclayout_set(struct nand_chip *nand, int idx)
+{
+	if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM)
Quoted 8 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 404

Quoted 7 lines
drivers/mtd/nand/davinci_nand.c +7 -0
+		return -1;
+
+	nand->ecc.layout = davinci_nand_ecclayouts[idx];
+
+	return 0;
+}
+#endif

Put a comment on this indicating what condition the #endif is ending.

Ok.

Quoted 15 lines

drivers/mtd/nand/davinci_nand.c void davinci_nand_init(struct nand_chip *nand) · line 735

Quoted 13 lines
drivers/mtd/nand/davinci_nand.c +4 -0
  static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode)
  {
  	u32 val;

@@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand)
  	nand->ecc.calculate = nand_davinci_4bit_calculate_ecc;
  	nand->ecc.correct = nand_davinci_4bit_correct_data;
  	nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc;
+#ifndef CONFIG_CMD_NAND_ECCLAYOUT
  	nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst;
+#else
+	nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst;
+#endif

Use "ifdef/else" rather than "ifndef/else".

Quoted 2 lines

Why does enabling layout selection change the default layout? Nothing in the changelog suggests that.

It's not correct. I will move it to board code.
I'll better do smth like the following:

In board config:
#define CONFIG_SYS_NAND_SELF_INIT in board config:

In nand davinci header:
#define NAND_DAVINCI_4BIT_LAYOUT 0
#define NAND_KEYSTONE_RBL_4BIT_LAYOUT 1

In board file:
int board_nand_init(struct nand_chip *chip)

Quoted 7 lines
{
     davinci_nand_init(chip);
     chip->ecc.layout =

board_nand_ecclayout_get_layout(NAND_KEYSTONE_RBL_4BIT_LAYOUT);

     return 0;
}

What does CONFIG_SYS_NAND_SELF_INIT have to do with this? Note that with that set, board_nand_init does not take an argument.

"board_nand_ecclayout_get_layout" is a bit long -- how about just "board_nand_get_ecclayout"?

-Scott

author
#13 lore raw

PATCH 2/2 mtd: nand: davinci: allow to change ecclayout by ecclayout command

Quoted 168 lines · Scott Wood
Quoted 167 lines · Ivan Khoronzhuk
Quoted 30 lines · Scott Wood
Quoted 20 lines · Ivan

From: WingMan Kwok <[email protected]>

This patch adds opportunity to change ecclayout of current nand device during runtime. So we can change the current nand device ecclayout using the "nand ecclayout set" command before writing the data to nand flash.

Signed-off-by Hao Zhang <[email protected]>
Signed-off-by WingMan Kwok <[email protected]>
Signed-off-by Ivan Khoronzhuk <[email protected]>

---
drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++

   1 file changed, 101 insertions(+)
drivers/mtd/nand/davinci_nand.c +0 -0
index 75b03a7..b33de0d 100644

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 310

Quoted 5 lines
drivers/mtd/nand/davinci_nand.c +2 -0
@@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = {
   #endif
   };
 
+#if defined(CONFIG_CMD_NAND_ECCLAYOUT)
+#if defined(CONFIG_SYS_NAND_PAGE_2K)

Undocumented CONFIG symbol (yes, I know it's not new)... Also redundant with CONFIG_SYS_NAND_PAGE_SIZE (though that's only documented in the context of SPL).

Quoted 13 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 322

Quoted 12 lines
drivers/mtd/nand/davinci_nand.c +12 -0
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 40,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+	},

Why the odd 2/8 pattern?

...I'll align.

Quoted 55 lines
Quoted 55 lines
drivers/mtd/nand/davinci_nand.c +55 -0
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+	},
+};
+#elif defined(CONFIG_SYS_NAND_PAGE_4K)
+static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = {
+	.eccbytes = 80,
+	.eccpos = {
+		6, 7,
+		8, 9, 10, 11, 12, 13, 14, 15,
+		22, 23,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		38, 39,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		54, 55,
+		56, 57, 58, 59, 60, 61, 62, 63,
+		70, 71,
+		72, 73, 74, 75, 76, 77, 78, 79,
+		86, 87,
+		88, 89, 90, 91, 92, 93, 94, 95,
+		102, 103,
+		104, 105, 106, 107, 108, 109, 110, 111,
+		118, 119,
+		120, 121, 122, 123, 124, 125, 126, 127,
+		},
+	.oobfree = {
+		{.offset = 2, .length = 4, },
+		{.offset = 16, .length = 6, },
+		{.offset = 32, .length = 6, },
+		{.offset = 48, .length = 6, },
+		{.offset = 64, .length = 6, },
+		{.offset = 80, .length = 6, },
+		{.offset = 96, .length = 6, },
+		{.offset = 112, .length = 6, },
+	},
+};
+#endif
+
+#define NAND_ECCLAYOUT_NUM 2
+
+struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = {
+	&nand_davinci_4bit_layout_oobfirst,
+	&nand_keystone_rbl_4bit_layout_oobfirst,
+};
+
+int board_nand_ecclayout_get_idx(struct nand_chip *nand,
+				 struct nand_ecclayout *p)
+{
+	int i;
+
+	if (!p)
+		return -1;
Quoted 8 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 384

Quoted 7 lines
drivers/mtd/nand/davinci_nand.c +7 -0
+
+	for (i = 0; i < NAND_ECCLAYOUT_NUM; i++)
+		if (davinci_nand_ecclayouts[i] == p)
+			return i;
+
+	return -1;
+}

Use ARRAY_SIZE().

Ok.

Quoted 13 lines
Quoted 13 lines
drivers/mtd/nand/davinci_nand.c +13 -0
+
+struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand,
+						       int idx)
+{
+	if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM))
+		return davinci_nand_ecclayouts[idx];
+	else
+		return NULL;
+}
+
+int board_nand_ecclayout_set(struct nand_chip *nand, int idx)
+{
+	if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM)
Quoted 8 lines

drivers/mtd/nand/davinci_nand.c static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { · line 404

Quoted 7 lines
drivers/mtd/nand/davinci_nand.c +7 -0
+		return -1;
+
+	nand->ecc.layout = davinci_nand_ecclayouts[idx];
+
+	return 0;
+}
+#endif

Put a comment on this indicating what condition the #endif is ending.

Ok.

Quoted 14 lines

drivers/mtd/nand/davinci_nand.c void davinci_nand_init(struct nand_chip *nand) · line 735

Quoted 12 lines
drivers/mtd/nand/davinci_nand.c +4 -0
   static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode)
   {
   	u32 val;

@@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand)
   	nand->ecc.calculate = nand_davinci_4bit_calculate_ecc;
   	nand->ecc.correct = nand_davinci_4bit_correct_data;
   	nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc;
+#ifndef CONFIG_CMD_NAND_ECCLAYOUT
   	nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst;
+#else
+	nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst;
+#endif

Use "ifdef/else" rather than "ifndef/else".

Quoted 2 lines

Why does enabling layout selection change the default layout? Nothing in the changelog suggests that.

It's not correct. I will move it to board code.
I'll better do smth like the following:

In board config:
#define CONFIG_SYS_NAND_SELF_INIT in board config:

In nand davinci header:
#define NAND_DAVINCI_4BIT_LAYOUT 0
#define NAND_KEYSTONE_RBL_4BIT_LAYOUT 1

In board file:
int board_nand_init(struct nand_chip *chip)

{
      davinci_nand_init(chip);
      chip->ecc.layout =

board_nand_ecclayout_get_layout(NAND_KEYSTONE_RBL_4BIT_LAYOUT);

Quoted 4 lines
Quoted 2 lines
      return 0;
}

What does CONFIG_SYS_NAND_SELF_INIT have to do with this? Note that with that set, board_nand_init does not take an argument.

Yes. Just seeing this while implementing...It's not needed.

Quoted 3 lines

"board_nand_ecclayout_get_layout" is a bit long -- how about just "board_nand_get_ecclayout"?

I will rename to board_nand_get_ecclayout

Signature · 2 lines
Regards,
Ivan Khoronzhuk