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

On 06/20/2014 04:00 AM, Scott Wood wrote:

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?

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

...

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

Yes.

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.

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.

...

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

Yes, It should be moved to board_nand_ecclayout_set()

...

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

...

• 	}
  

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

Ok.

...

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

Yes.

...

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.

Ok. Thanks! I will repost soon.

-Scott