From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ivan Khoronzhuk Date: Fri, 20 Jun 2014 16:29:54 +0300 Subject: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command In-Reply-To: <20140620010012.GA5906@home.buserror.net> References: <1400264797-3593-2-git-send-email-ivan.khoronzhuk@ti.com> <20140620010012.GA5906@home.buserror.net> Message-ID: <53A43752.2080400@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> >> 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 >> Signed-off-by: Ivan Khoronzhuk >> >> --- >> 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 |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 -- Regards, Ivan Khoronzhuk