Re: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command

On Fri, May 16, 2014 at 09:26:36PM +0300, Khoronzhuk, Ivan wrote:

From: WingMan Kwok w-kwok2@ti.com

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 w-kwok2@ti.com Signed-off-by: Ivan Khoronzhuk ivan.khoronzhuk@ti.com

---

common/cmd_nand.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++- include/nand.h | 10 +++++ 2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c index 04ab0f1..7cbe6fc 100644 --- a/common/cmd_nand.c +++ b/common/cmd_nand.c @@ -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]);
  

• }

Why 9?

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

  }

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?

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

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

• 	} 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).

• 	}
  

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

• }
  

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

• 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

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

diff --git a/include/nand.h b/include/nand.h index fc735d1..0e8a093 100644 --- a/include/nand.h +++ b/include/nand.h @@ -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

Don't ifdef prototypes.

-Scott