[U-Boot] [RFC PATCH 0/4] cmd: Simplify support for sub-commands

[U-Boot] [RFC PATCH 0/4] cmd: Simplify support for sub-commands

[Boris Brezillon]

Hello,

This patch series aims at simplifying the command parsing logic done in
pretty much all the cmd/foo.c files by adding a few macros that help
defining sub-commands attach to the main entry point.

When you use those macros you also get sub-command auto-completion for
free (the rest of the auto-completion still has to be done manually).

Support for several levels of sub commands is not supported but can
easily be added if needed.

Some details about the patches:

- Patch 1 is a fix for the auto-completion code that I had to do have
  auto-completion in the mtd command working correctly (I can submit it
  separately if needed)
- Patch 2 is exposing a function to ease support of auto-completion of
  sub-commands
- Patch 3 is adding a set of macros to easily declare the sub-commands
  attached to the main command
- Patch 4 is making use of this new infrastructure in cmd/mtd.c.

Regards,

Boris

Boris Brezillon (4):
  common: command: Fix command auto-completion
  common: command: Expose a generic helper to auto-complete sub commands
  command: commands: Add macros to declare commands with subcmds
  cmd: mtd: Use the subcmd infrastructure to declare mtd sub-commands

 cmd/mtd.c         | 475 +++++++++++++++++++++++++++-------------------
 common/command.c  |  32 +++-
 include/command.h |  54 ++++++
 3 files changed, 360 insertions(+), 201 deletions(-)

-- 
2.17.1

[U-Boot] [RFC PATCH 1/4] common: command: Fix command auto-completion

[Boris Brezillon]

When auto-completing command arguments, the last argument is not
necessarily the one we need to auto-complete. When the last character is
a space, a tab or '\0' what we want instead is list all possible values,
or if there's only one possible value, place this value on the command
line instead of trying to suffix the last valid argument with missing
chars.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 common/command.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/common/command.c b/common/command.c
index 2433a89e0a8e..435824356b50 100644
--- a/common/command.c
+++ b/common/command.c
@@ -362,13 +362,21 @@ int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp)
 	sep = NULL;
 	seplen = 0;
 	if (i == 1) { /* one match; perfect */
-		k = strlen(argv[argc - 1]);
+		if (last_char != '\0' && !isblank(last_char))
+			k = strlen(argv[argc - 1]);
+		else
+			k = 0;
+
 		s = cmdv[0] + k;
 		len = strlen(s);
 		sep = " ";
 		seplen = 1;
 	} else if (i > 1 && (j = find_common_prefix(cmdv)) != 0) { /* more */
-		k = strlen(argv[argc - 1]);
+		if (last_char != '\0' && !isblank(last_char))
+			k = strlen(argv[argc - 1]);
+		else
+			k = 0;
+
 		j -= k;
 		if (j > 0) {
 			s = cmdv[0] + k;
-- 
2.17.1

[U-Boot] [RFC PATCH 2/4] common: command: Expose a generic helper to auto-complete sub commands

[Boris Brezillon]

Some commands have a table of sub-commands. With a minor adjustments,
complete_cmdv() is able to provide auto-completion for sub-commands
(it's just about passing the table of commands instead of taking the
global one).
We rename this function into complete_subcmd() and implement
complete_cmdv() as a wrapper around complete_subcmdv().

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 common/command.c  | 20 ++++++++++++++++----
 include/command.h |  3 +++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/common/command.c b/common/command.c
index 435824356b50..e13cb47ac18b 100644
--- a/common/command.c
+++ b/common/command.c
@@ -161,11 +161,11 @@ int var_complete(int argc, char * const argv[], char last_char, int maxv, char *
 
 /*************************************************************************************/
 
-static int complete_cmdv(int argc, char * const argv[], char last_char, int maxv, char *cmdv[])
+int complete_subcmdv(cmd_tbl_t *cmdtp, int count, int argc,
+		     char * const argv[], char last_char,
+		     int maxv, char *cmdv[])
 {
 #ifdef CONFIG_CMDLINE
-	cmd_tbl_t *cmdtp = ll_entry_start(cmd_tbl_t, cmd);
-	const int count = ll_entry_count(cmd_tbl_t, cmd);
 	const cmd_tbl_t *cmdend = cmdtp + count;
 	const char *p;
 	int len, clen;
@@ -193,7 +193,7 @@ static int complete_cmdv(int argc, char * const argv[], char last_char, int maxv
 
 	/* more than one arg or one but the start of the next */
 	if (argc > 1 || last_char == '\0' || isblank(last_char)) {
-		cmdtp = find_cmd(argv[0]);
+		cmdtp = find_cmd_tbl(argv[0], cmdtp, count);
 		if (cmdtp == NULL || cmdtp->complete == NULL) {
 			cmdv[0] = NULL;
 			return 0;
@@ -238,6 +238,18 @@ static int complete_cmdv(int argc, char * const argv[], char last_char, int maxv
 #endif
 }
 
+static int complete_cmdv(int argc, char * const argv[], char last_char,
+			 int maxv, char *cmdv[])
+{
+#ifdef CONFIG_CMDLINE
+	return complete_subcmdv(ll_entry_start(cmd_tbl_t, cmd),
+				ll_entry_count(cmd_tbl_t, cmd), argc, argv,
+				last_char, maxv, cmdv);
+#else
+	return 0;
+#endif
+}
+
 static int make_argv(char *s, int argvsz, char *argv[])
 {
 	int argc = 0;
diff --git a/include/command.h b/include/command.h
index 200c7a5e9f4e..89efcecfa926 100644
--- a/include/command.h
+++ b/include/command.h
@@ -54,6 +54,9 @@ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
 	      flag, int argc, char * const argv[]);
 cmd_tbl_t *find_cmd(const char *cmd);
 cmd_tbl_t *find_cmd_tbl (const char *cmd, cmd_tbl_t *table, int table_len);
+int complete_subcmdv(cmd_tbl_t *cmdtp, int count, int argc,
+		     char * const argv[], char last_char, int maxv,
+		     char *cmdv[]);
 
 extern int cmd_usage(const cmd_tbl_t *cmdtp);
 
-- 
2.17.1

[U-Boot] [RFC PATCH 3/4] command: commands: Add macros to declare commands with subcmds

[Boris Brezillon]

Most cmd/xxx.c source files expose several commands through a single
entry point. Some of them are doing the sub-command parsing manually in
their do_<cmd>() function, others are declaring a table of sub-commands
and then use find_cmd_tbl() to delegate the request to the sub command
handler.

In both case, the amount of code to do that is not negligible and
repetitive, not to mention that almost no commands are implementing
a auto-completion hook, which means most u-boot lack auto-completion.

Provide several macros to easily define sub-commands and commands
exposing such sub-commands.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 include/command.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/include/command.h b/include/command.h
index 89efcecfa926..d83647f9f010 100644
--- a/include/command.h
+++ b/include/command.h
@@ -187,6 +187,44 @@ int board_run_command(const char *cmdline);
 # define _CMD_HELP(x)
 #endif
 
+#define U_BOOT_SUBCMDS_DO_CMD(_cmdname, _maxargs)			\
+	static int do_##_cmdname(cmd_tbl_t *cmdtp, int flag, int argc,	\
+				 char * const argv[])			\
+	{								\
+		cmd_tbl_t *subcmd;					\
+									\
+		/* We need at least the cmd and subcmd names. */	\
+		if (argc < 2 || argc > CONFIG_SYS_MAXARGS)		\
+			return CMD_RET_USAGE;				\
+									\
+		subcmd = find_cmd_tbl(argv[1], _cmdname##_subcmds,	\
+				      ARRAY_SIZE(_cmdname##_subcmds));	\
+		if (!subcmd || argc - 1 > subcmd->maxargs)		\
+			return CMD_RET_USAGE;				\
+									\
+		return subcmd->cmd(subcmd, flag, argc - 1, argv + 1);	\
+	}
+
+#ifdef CONFIG_AUTO_COMPLETE
+#define U_BOOT_SUBCMDS_COMPLETE(_cmdname, _maxargs)			\
+	static int complete_##_cmdname(int argc, char * const argv[],	\
+				       char last_char, int maxv,	\
+				       char *cmdv[])			\
+	{								\
+		return complete_subcmdv(_cmdname##_subcmds,		\
+					ARRAY_SIZE(_cmdname##_subcmds),	\
+					argc - 1, argv + 1, last_char,	\
+					maxv, cmdv);			\
+	}
+#else
+#define U_BOOT_SUBCMDS_COMPLETE(_cmdname, _maxargs)
+#endif
+
+#define U_BOOT_SUBCMDS(_cmdname, _maxargs, ...)				\
+	static cmd_tbl_t _cmdname##_subcmds[] = { __VA_ARGS__ };	\
+	U_BOOT_SUBCMDS_DO_CMD(_cmdname, _maxargs)			\
+	U_BOOT_SUBCMDS_COMPLETE(_cmdname, _maxargs)
+
 #ifdef CONFIG_CMDLINE
 #define U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd,		\
 				_usage, _help, _comp)			\
@@ -227,4 +265,17 @@ int board_run_command(const char *cmdline);
 	U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd,		\
 					_usage, _help, NULL)
 
+#define U_BOOT_SUBCMD_MKENT_COMPLETE(_name, _maxargs, _do_cmd, _comp)	\
+	U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, 0, _do_cmd, "", "",	\
+				  _comp)
+
+#define U_BOOT_SUBCMD_MKENT(_name, _maxargs, _do_cmd)			\
+	U_BOOT_SUBCMD_MKENT_COMPLETE(_name, _maxargs, _do_cmd, NULL)
+
+#define U_BOOT_CMD_WITH_SUBCMDS(_name, _maxargs, _rep, _usage, _help,	\
+				...)					\
+	U_BOOT_SUBCMDS(_name, _maxargs, __VA_ARGS__)			\
+	U_BOOT_CMD_COMPLETE(_name, _maxargs, _rep, do_##_name, _usage,	\
+			    _help, complete_##_name)
+
 #endif	/* __COMMAND_H */
-- 
2.17.1

[U-Boot] [RFC PATCH 4/4] cmd: mtd: Use the subcmd infrastructure to declare mtd sub-commands

[Boris Brezillon]

It's way simpler this way, and we also gain auto-completion support for
free (MTD name auto-completion has been added with do_mtd_name_complete())

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 cmd/mtd.c | 475 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 280 insertions(+), 195 deletions(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 614222398467..c1617329d5dc 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -15,6 +15,22 @@
 #include <mapmem.h>
 #include <mtd.h>
 
+#include <linux/ctype.h>
+
+static struct mtd_info *get_mtd_by_name(const char *name)
+{
+	struct mtd_info *mtd;
+
+	mtd_probe_devices();
+
+	mtd = get_mtd_device_nm(name);
+	if (IS_ERR_OR_NULL(mtd))
+		printf("MTD device %s not found, ret %ld\n", name,
+		       PTR_ERR(mtd));
+
+	return mtd;
+}
+
 static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
 {
 	do_div(len, mtd->writesize);
@@ -177,7 +193,7 @@ static bool mtd_oob_write_is_empty(struct mtd_oob_ops *op)
 	return true;
 }
 
-static int do_mtd_list(void)
+static int do_mtd_list(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	struct mtd_info *mtd;
 	int dev_nb = 0;
@@ -221,229 +237,287 @@ static int mtd_special_write_oob(struct mtd_info *mtd, u64 off,
 	return ret;
 }
 
-static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+static int do_mtd_io(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
+	bool dump, read, raw, woob, write_empty_pages, has_pages = false;
+	u64 start_off, off, len, remaining, default_len;
+	struct mtd_oob_ops io_op = {};
+	uint user_addr = 0, npages;
+	const char *cmd = argv[0];
 	struct mtd_info *mtd;
-	const char *cmd;
-	char *mtd_name;
+	u32 oob_len;
+	u8 *buf;
+	int ret;
 
-	/* All MTD commands need at least two arguments */
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	/* Parse the command name and its optional suffixes */
-	cmd = argv[1];
-
-	/* List the MTD devices if that is what the user wants */
-	if (strcmp(cmd, "list") == 0)
-		return do_mtd_list();
-
-	/*
-	 * The remaining commands require also@least a device ID.
-	 * Check the selected device is valid. Ensure it is probed.
-	 */
-	if (argc < 3)
-		return CMD_RET_USAGE;
-
-	mtd_name = argv[2];
-	mtd_probe_devices();
-	mtd = get_mtd_device_nm(mtd_name);
-	if (IS_ERR_OR_NULL(mtd)) {
-		printf("MTD device %s not found, ret %ld\n",
-		       mtd_name, PTR_ERR(mtd));
+	mtd = get_mtd_by_name(argv[1]);
+	if (IS_ERR_OR_NULL(mtd))
 		return CMD_RET_FAILURE;
+
+	if (mtd->type == MTD_NANDFLASH || mtd->type == MTD_MLCNANDFLASH)
+		has_pages = true;
+
+	dump = !strncmp(cmd, "dump", 4);
+	read = dump || !strncmp(cmd, "read", 4);
+	raw = strstr(cmd, ".raw");
+	woob = strstr(cmd, ".oob");
+	write_empty_pages = !has_pages || strstr(cmd, ".dontskipff");
+
+	argc -= 2;
+	argv += 2;
+
+	if (!dump) {
+		if (!argc) {
+			ret = CMD_RET_USAGE;
+			goto out_put_mtd;
+		}
+
+		user_addr = simple_strtoul(argv[0], NULL, 16);
+		argc--;
+		argv++;
 	}
-	put_mtd_device(mtd);
 
-	argc -= 3;
-	argv += 3;
+	start_off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
+	if (!mtd_is_aligned_with_min_io_size(mtd, start_off)) {
+		printf("Offset not aligned with a page (0x%x)\n",
+		       mtd->writesize);
+		ret = CMD_RET_FAILURE;
+		goto out_put_mtd;
+	}
 
-	/* Do the parsing */
-	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "dump", 4) ||
-	    !strncmp(cmd, "write", 5)) {
-		bool has_pages = mtd->type == MTD_NANDFLASH ||
-				 mtd->type == MTD_MLCNANDFLASH;
-		bool dump, read, raw, woob, write_empty_pages;
-		struct mtd_oob_ops io_op = {};
-		uint user_addr = 0, npages;
-		u64 start_off, off, len, remaining, default_len;
-		u32 oob_len;
-		u8 *buf;
-		int ret;
+	default_len = dump ? mtd->writesize : mtd->size;
+	len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : default_len;
+	if (!mtd_is_aligned_with_min_io_size(mtd, len)) {
+		len = round_up(len, mtd->writesize);
+		printf("Size not on a page boundary (0x%x), rounding to 0x%llx\n",
+		       mtd->writesize, len);
+	}
 
-		dump = !strncmp(cmd, "dump", 4);
-		read = dump || !strncmp(cmd, "read", 4);
-		raw = strstr(cmd, ".raw");
-		woob = strstr(cmd, ".oob");
-		write_empty_pages = !has_pages || strstr(cmd, ".dontskipff");
+	remaining = len;
+	npages = mtd_len_to_pages(mtd, len);
+	oob_len = woob ? npages * mtd->oobsize : 0;
 
-		if (!dump) {
-			if (!argc)
-				return CMD_RET_USAGE;
+	if (dump)
+		buf = kmalloc(len + oob_len, GFP_KERNEL);
+	else
+		buf = map_sysmem(user_addr, 0);
 
-			user_addr = simple_strtoul(argv[0], NULL, 16);
-			argc--;
-			argv++;
-		}
+	if (!buf) {
+		printf("Could not map/allocate the user buffer\n");
+		ret = CMD_RET_FAILURE;
+		goto out_put_mtd;
+	}
 
-		start_off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
-		if (!mtd_is_aligned_with_min_io_size(mtd, start_off)) {
-			printf("Offset not aligned with a page (0x%x)\n",
-			       mtd->writesize);
-			return CMD_RET_FAILURE;
-		}
+	if (has_pages)
+		printf("%s %lld byte(s) (%d page(s)) at offset 0x%08llx%s%s%s\n",
+		       read ? "Reading" : "Writing", len, npages, start_off,
+		       raw ? " [raw]" : "", woob ? " [oob]" : "",
+		       !read && write_empty_pages ? " [dontskipff]" : "");
+	else
+		printf("%s %lld byte(s) at offset 0x%08llx\n",
+		       read ? "Reading" : "Writing", len, start_off);
 
-		default_len = dump ? mtd->writesize : mtd->size;
-		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) :
-				 default_len;
-		if (!mtd_is_aligned_with_min_io_size(mtd, len)) {
-			len = round_up(len, mtd->writesize);
-			printf("Size not on a page boundary (0x%x), rounding to 0x%llx\n",
-			       mtd->writesize, len);
-		}
+	io_op.mode = raw ? MTD_OPS_RAW : MTD_OPS_AUTO_OOB;
+	io_op.len = has_pages ? mtd->writesize : len;
+	io_op.ooblen = woob ? mtd->oobsize : 0;
+	io_op.datbuf = buf;
+	io_op.oobbuf = woob ? &buf[len] : NULL;
 
-		remaining = len;
-		npages = mtd_len_to_pages(mtd, len);
-		oob_len = woob ? npages * mtd->oobsize : 0;
+	/* Search for the first good block after the given offset */
+	off = start_off;
+	while (mtd_block_isbad(mtd, off))
+		off += mtd->erasesize;
 
-		if (dump)
-			buf = kmalloc(len + oob_len, GFP_KERNEL);
-		else
-			buf = map_sysmem(user_addr, 0);
-
-		if (!buf) {
-			printf("Could not map/allocate the user buffer\n");
-			return CMD_RET_FAILURE;
-		}
-
-		if (has_pages)
-			printf("%s %lld byte(s) (%d page(s)) at offset 0x%08llx%s%s%s\n",
-			       read ? "Reading" : "Writing", len, npages, start_off,
-			       raw ? " [raw]" : "", woob ? " [oob]" : "",
-			       !read && write_empty_pages ? " [dontskipff]" : "");
-		else
-			printf("%s %lld byte(s) at offset 0x%08llx\n",
-			       read ? "Reading" : "Writing", len, start_off);
-
-		io_op.mode = raw ? MTD_OPS_RAW : MTD_OPS_AUTO_OOB;
-		io_op.len = has_pages ? mtd->writesize : len;
-		io_op.ooblen = woob ? mtd->oobsize : 0;
-		io_op.datbuf = buf;
-		io_op.oobbuf = woob ? &buf[len] : NULL;
-
-		/* Search for the first good block after the given offset */
-		off = start_off;
-		while (mtd_block_isbad(mtd, off))
+	/* Loop over the pages to do the actual read/write */
+	while (remaining) {
+		/* Skip the block if it is bad */
+		if (mtd_is_aligned_with_block_size(mtd, off) &&
+		    mtd_block_isbad(mtd, off)) {
 			off += mtd->erasesize;
-
-		/* Loop over the pages to do the actual read/write */
-		while (remaining) {
-			/* Skip the block if it is bad */
-			if (mtd_is_aligned_with_block_size(mtd, off) &&
-			    mtd_block_isbad(mtd, off)) {
-				off += mtd->erasesize;
-				continue;
-			}
-
-			if (read)
-				ret = mtd_read_oob(mtd, off, &io_op);
-			else
-				ret = mtd_special_write_oob(mtd, off, &io_op,
-							    write_empty_pages,
-							    woob);
-
-			if (ret) {
-				printf("Failure while %s at offset 0x%llx\n",
-				       read ? "reading" : "writing", off);
-				return CMD_RET_FAILURE;
-			}
-
-			off += io_op.retlen;
-			remaining -= io_op.retlen;
-			io_op.datbuf += io_op.retlen;
-			io_op.oobbuf += io_op.oobretlen;
+			continue;
 		}
 
-		if (!ret && dump)
-			mtd_dump_device_buf(mtd, start_off, buf, len, woob);
-
-		if (dump)
-			kfree(buf);
+		if (read)
+			ret = mtd_read_oob(mtd, off, &io_op);
 		else
-			unmap_sysmem(buf);
+			ret = mtd_special_write_oob(mtd, off, &io_op,
+						    write_empty_pages, woob);
 
 		if (ret) {
-			printf("%s on %s failed with error %d\n",
-			       read ? "Read" : "Write", mtd->name, ret);
-			return CMD_RET_FAILURE;
+			printf("Failure while %s at offset 0x%llx\n",
+			       read ? "reading" : "writing", off);
+			break;
 		}
 
-	} else if (!strcmp(cmd, "erase")) {
-		bool scrub = strstr(cmd, ".dontskipbad");
-		struct erase_info erase_op = {};
-		u64 off, len;
-		int ret;
-
-		off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
-		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->size;
-
-		if (!mtd_is_aligned_with_block_size(mtd, off)) {
-			printf("Offset not aligned with a block (0x%x)\n",
-			       mtd->erasesize);
-			return CMD_RET_FAILURE;
-		}
-
-		if (!mtd_is_aligned_with_block_size(mtd, len)) {
-			printf("Size not a multiple of a block (0x%x)\n",
-			       mtd->erasesize);
-			return CMD_RET_FAILURE;
-		}
-
-		printf("Erasing 0x%08llx ... 0x%08llx (%d eraseblock(s))\n",
-		       off, off + len - 1, mtd_div_by_eb(len, mtd));
-
-		erase_op.mtd = mtd;
-		erase_op.addr = off;
-		erase_op.len = len;
-		erase_op.scrub = scrub;
-
-		while (erase_op.len) {
-			ret = mtd_erase(mtd, &erase_op);
-
-			/* Abort if its not a bad block error */
-			if (ret != -EIO)
-				break;
-
-			printf("Skipping bad block at 0x%08llx\n",
-			       erase_op.fail_addr);
-
-			/* Skip bad block and continue behind it */
-			erase_op.len -= erase_op.fail_addr - erase_op.addr;
-			erase_op.len -= mtd->erasesize;
-			erase_op.addr = erase_op.fail_addr + mtd->erasesize;
-		}
-
-		if (ret && ret != -EIO)
-			return CMD_RET_FAILURE;
-	} else if (!strcmp(cmd, "bad")) {
-		loff_t off;
-
-		if (!mtd_can_have_bb(mtd)) {
-			printf("Only NAND-based devices can have bad blocks\n");
-			return CMD_RET_SUCCESS;
-		}
-
-		printf("MTD device %s bad blocks list:\n", mtd->name);
-		for (off = 0; off < mtd->size; off += mtd->erasesize)
-			if (mtd_block_isbad(mtd, off))
-				printf("\t0x%08llx\n", off);
-	} else {
-		return CMD_RET_USAGE;
+		off += io_op.retlen;
+		remaining -= io_op.retlen;
+		io_op.datbuf += io_op.retlen;
+		io_op.oobbuf += io_op.oobretlen;
 	}
 
+	if (!ret && dump)
+		mtd_dump_device_buf(mtd, start_off, buf, len, woob);
+
+	if (dump)
+		kfree(buf);
+	else
+		unmap_sysmem(buf);
+
+	if (ret) {
+		printf("%s on %s failed with error %d\n",
+		       read ? "Read" : "Write", mtd->name, ret);
+		ret = CMD_RET_FAILURE;
+	} else {
+		ret = CMD_RET_SUCCESS;
+	}
+
+out_put_mtd:
+	put_mtd_device(mtd);
+
+	return ret;
+}
+
+static int do_mtd_erase(cmd_tbl_t *cmdtp, int flag, int argc,
+			char * const argv[])
+{
+	struct erase_info erase_op = {};
+	struct mtd_info *mtd;
+	u64 off, len;
+	bool scrub;
+	int ret;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	mtd = get_mtd_by_name(argv[1]);
+	if (IS_ERR_OR_NULL(mtd))
+		return CMD_RET_FAILURE;
+
+	scrub = strstr(argv[0], ".dontskipbad");
+
+	argc -= 2;
+	argv += 2;
+
+	off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
+	len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->size;
+
+	if (!mtd_is_aligned_with_block_size(mtd, off)) {
+		printf("Offset not aligned with a block (0x%x)\n",
+		       mtd->erasesize);
+		ret = CMD_RET_FAILURE;
+		goto out_put_mtd;
+	}
+
+	if (!mtd_is_aligned_with_block_size(mtd, len)) {
+		printf("Size not a multiple of a block (0x%x)\n",
+		       mtd->erasesize);
+		ret = CMD_RET_FAILURE;
+		goto out_put_mtd;
+	}
+
+	printf("Erasing 0x%08llx ... 0x%08llx (%d eraseblock(s))\n",
+	       off, off + len - 1, mtd_div_by_eb(len, mtd));
+
+	erase_op.mtd = mtd;
+	erase_op.addr = off;
+	erase_op.len = len;
+	erase_op.scrub = scrub;
+
+	while (erase_op.len) {
+		ret = mtd_erase(mtd, &erase_op);
+
+		/* Abort if its not a bad block error */
+		if (ret != -EIO)
+			break;
+
+		printf("Skipping bad block at 0x%08llx\n", erase_op.fail_addr);
+
+		/* Skip bad block and continue behind it */
+		erase_op.len -= erase_op.fail_addr - erase_op.addr;
+		erase_op.len -= mtd->erasesize;
+		erase_op.addr = erase_op.fail_addr + mtd->erasesize;
+	}
+
+	if (ret && ret != -EIO)
+		ret = CMD_RET_FAILURE;
+	else
+		ret = CMD_RET_SUCCESS;
+
+out_put_mtd:
+	put_mtd_device(mtd);
+
+	return ret;
+}
+
+static int do_mtd_bad(cmd_tbl_t *cmdtp, int flag, int argc,
+		      char * const argv[])
+{
+	struct mtd_info *mtd;
+	loff_t off;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	mtd = get_mtd_by_name(argv[1]);
+	if (IS_ERR_OR_NULL(mtd))
+		return CMD_RET_FAILURE;
+
+	if (!mtd_can_have_bb(mtd)) {
+		printf("Only NAND-based devices can have bad blocks\n");
+		goto out_put_mtd;
+	}
+
+	printf("MTD device %s bad blocks list:\n", mtd->name);
+	for (off = 0; off < mtd->size; off += mtd->erasesize) {
+		if (mtd_block_isbad(mtd, off))
+			printf("\t0x%08llx\n", off);
+	}
+
+out_put_mtd:
+	put_mtd_device(mtd);
+
 	return CMD_RET_SUCCESS;
 }
 
+#ifdef CONFIG_AUTO_COMPLETE
+static int do_mtd_name_complete(int argc, char * const argv[],
+				char last_char, int maxv, char *cmdv[])
+{
+	int len = 0, n_found = 0;
+	struct mtd_info *mtd;
+
+	argc--;
+	argv++;
+
+	if (argc > 1 ||
+	    (argc == 1 && (last_char == '\0' || isblank(last_char))))
+		return 0;
+
+	if (argc)
+		len = strlen(argv[0]);
+
+	mtd_for_each_device(mtd) {
+		if (argc &&
+		    (len > strlen(mtd->name) ||
+		     strncmp(argv[0], mtd->name, len)))
+			continue;
+
+		if (n_found >= maxv - 2) {
+			cmdv[n_found++] = "...";
+			break;
+		}
+
+		cmdv[n_found++] = mtd->name;
+	}
+
+	cmdv[n_found] = NULL;
+
+	return n_found;
+}
+#endif /* CONFIG_AUTO_COMPLETE */
+
 static char mtd_help_text[] =
 #ifdef CONFIG_SYS_LONGHELP
 	"- generic operations on memory technology devices\n\n"
@@ -470,4 +544,15 @@ static char mtd_help_text[] =
 #endif
 	"";
 
-U_BOOT_CMD(mtd, 10, 1, do_mtd, "MTD utils", mtd_help_text);
+U_BOOT_CMD_WITH_SUBCMDS(mtd, 10, 1, "MTD utils", mtd_help_text,
+		U_BOOT_SUBCMD_MKENT(list, 1, do_mtd_list),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(read, 5, do_mtd_io,
+					     do_mtd_name_complete),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(write, 5, do_mtd_io,
+					     do_mtd_name_complete),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(dump, 4, do_mtd_io,
+					     do_mtd_name_complete),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(erase, 4, do_mtd_erase,
+					     do_mtd_name_complete),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(bad, 1, do_mtd_bad,
+					     do_mtd_name_complete))
-- 
2.17.1

[U-Boot] [RFC PATCH 0/4] cmd: Simplify support for sub-commands

[Boris Brezillon]

+Tom, Marek as I realize I only sent this to the ML, and I'm actually
expecting feedback from maintainers. Let me know if you think I should
Cc other people.

On Fri, 16 Nov 2018 23:38:08 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hello,
> 
> This patch series aims at simplifying the command parsing logic done in
> pretty much all the cmd/foo.c files by adding a few macros that help
> defining sub-commands attach to the main entry point.

For those who are interested, I started converting some uboot cmds to
this approach [1]. Most of them are easily converted (I'd say 3/4 or
the existing U_BOOT_CMD_MKENT() users), the remaining ones need extra
care. But before I even consider sending a v2 (possibly including
those conversion patches), I'd like to have some feedback on the general
approach (whether it's something u-boot maintainers are interested in
or not).

Thanks,

Boris

> 
> When you use those macros you also get sub-command auto-completion for
> free (the rest of the auto-completion still has to be done manually).
> 
> Support for several levels of sub commands is not supported but can
> easily be added if needed.
> 
> Some details about the patches:
> 
> - Patch 1 is a fix for the auto-completion code that I had to do have
>   auto-completion in the mtd command working correctly (I can submit it
>   separately if needed)
> - Patch 2 is exposing a function to ease support of auto-completion of
>   sub-commands
> - Patch 3 is adding a set of macros to easily declare the sub-commands
>   attached to the main command
> - Patch 4 is making use of this new infrastructure in cmd/mtd.c.
> 
> Regards,
> 
> Boris
> 
> Boris Brezillon (4):
>   common: command: Fix command auto-completion
>   common: command: Expose a generic helper to auto-complete sub commands
>   command: commands: Add macros to declare commands with subcmds
>   cmd: mtd: Use the subcmd infrastructure to declare mtd sub-commands
> 
>  cmd/mtd.c         | 475 +++++++++++++++++++++++++++-------------------
>  common/command.c  |  32 +++-
>  include/command.h |  54 ++++++
>  3 files changed, 360 insertions(+), 201 deletions(-)
> 

[1]https://github.com/bbrezillon/u-boot/commits/sub-cmds