[U-Boot] [PATCH v2 20/21] cmd: mtd: add 'mtd' command

[Boris Brezillon]

On Wed, 11 Jul 2018 17:25:28 +0200
Miquel Raynal <miquel.raynal at bootlin.com> wrote:

> +
> +static void mtd_show_device(struct mtd_info *mtd)
> +{
> +	printf("* %s", mtd->name);

Printing the device type might be interesting? And maybe also the size,
writesize, oobsize and erasesize?

> +	if (mtd->dev)
> +		printf(" [device: %s] [parent: %s] [driver: %s]",
> +		       mtd->dev->name, mtd->dev->parent->name,
> +		       mtd->dev->driver->name);
> +
> +	printf("\n");
> +}


> +static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	struct mtd_info *mtd;
> +	const char *cmd;
> +	char *mtdname;
> +	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 at least a device ID.
> +	 * Check the selected device is valid. Ensure it is probed.
> +	 */
> +	if (argc < 3)
> +		return CMD_RET_USAGE;
> +
> +	mtdname = argv[2];
> +	mtd = get_mtd_device_nm(mtdname);
> +	if (IS_ERR_OR_NULL(mtd)) {
> +		mtd_probe_devices_dm();
> +		mtd = get_mtd_device_nm(mtdname);
> +		if (IS_ERR_OR_NULL(mtd)) {
> +			printf("MTD device %s not found, ret %ld\n",
> +			       mtdname, PTR_ERR(mtd));
> +			return 1;
> +		}
> +	}
> +
> +	argc -= 3;
> +	argv += 3;
> +
> +	/* Do the parsing */
> +	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "dump", 4) ||
> +	    !strncmp(cmd, "write", 5)) {
> +		struct mtd_oob_ops io_op = {};
> +		bool dump, read, raw, woob;
> +		uint user_addr = 0;
> +		uint nb_pages;
> +		u8 *buf;
> +		u64 off, len;
> +		u32 oob_len;
> +
> +		dump = !strncmp(cmd, "dump", 4);
> +		read = dump || !strncmp(cmd, "read", 4);
> +		raw = strstr(cmd, ".raw");
> +		woob = strstr(cmd, ".oob");
> +
> +		if (!dump) {
> +			if (!argc)
> +				return CMD_RET_USAGE;
> +
> +			user_addr = simple_strtoul(argv[0], NULL, 16);
> +			argc--;
> +			argv++;
> +		}
> +
> +		off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
> +		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->writesize;
> +		nb_pages = mtd_len_to_pages(mtd, len);
> +		oob_len = woob ? nb_pages * mtd->oobsize : 0;
> +
> +		if ((u32)off % mtd->writesize) {
> +			printf("Offset not aligned with a page (0x%x)\n",
> +			       mtd->writesize);
> +			return -EINVAL;
> +		}
> +
> +		if ((u32)len % mtd->writesize) {
> +			printf("Size not a multiple of a page (0x%x)\n",
> +			       mtd->writesize);
> +			return -EINVAL;
> +		}
> +
> +		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 -ENOMEM;
> +		}
> +
> +		printf("%s %lldB (%d page(s)) at offset 0x%08llx%s%s\n",
> +		       read ? "Reading" : "Writing", len, nb_pages, off,
> +		       raw ? " [raw]" : "", woob ? " [oob]" : "");
> +
> +		io_op.mode = raw ? MTD_OPS_RAW : MTD_OPS_AUTO_OOB;
> +		io_op.len = len;
> +		io_op.ooblen = oob_len;
> +		io_op.datbuf = buf;
> +		io_op.oobbuf = woob ? &buf[len] : NULL;
> +
> +		if (read)
> +			ret = mtd_read_oob(mtd, off, &io_op);
> +		else
> +			ret = mtd_write_oob(mtd, off, &io_op);
> +
> +		if (ret) {
> +			printf("%s on %s failed with error %d\n",
> +			       read ? "Read" : "Write", mtd->name, ret);
> +			return ret;
> +		}
> +
> +		if (dump) {
> +			uint page;
> +
> +			for (page = 0; page < nb_pages; page++) {
> +				u64 data_off = page * mtd->writesize;
> +				printf("\nDump %d data bytes from 0x%08llx:\n",
> +				       mtd->writesize, data_off);
> +				mtd_dump_buf(buf, mtd->writesize, data_off);
> +
> +				if (woob) {
> +					u64 oob_off = page * mtd->oobsize;
> +					printf("Dump %d OOB bytes from page at 0x%08llx:\n",
> +					       mtd->oobsize, data_off);
> +					mtd_dump_buf(&buf[len + oob_off],
> +						     mtd->oobsize, 0);
> +				}
> +			}
> +		}
> +
> +		if (dump)
> +			kfree(buf);
> +		else
> +			unmap_sysmem(buf);
> +
> +	} else if (!strcmp(cmd, "erase")) {
> +		bool scrub = strstr(cmd, ".dontskipbad");
> +		bool full_erase = strstr(cmd, ".chip");

Again, I think .chip extension is not needed. Just consider a full erase
is requested when offset and length are not provided. Plus, chip is
misleading since I guess mtd partitions are also exposed as mtd devices,
and could thus be erased with the .chip extension as well.

> +		struct erase_info erase_op = {};
> +		u64 off, len;
> +
> +		off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
> +		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->erasesize;

Hm. Are we sure we want the default to be 1 eraseblock? Shouldn't we
consider that missing size means "erase up to the MTD device end". Same
goes for the read/write accesses, but maybe not for dump where dumping
a single page makes more sense.

> +
> +		if (full_erase) {
> +			off = 0;
> +			len = mtd->size;
> +		}
> +
> +		if ((u32)off % mtd->erasesize) {

Sounds dangerous. We have 8GB NANDs on sunxi platforms...

> +			printf("Offset not aligned with a block (0x%x)\n",
> +			       mtd->erasesize);
> +			return -EINVAL;
> +		}
> +
> +		if ((u32)len % mtd->erasesize) {

Same here. I guess there's a do_div() in uboot.

> +			printf("Size not a multiple of a block (0x%x)\n",
> +			       mtd->erasesize);
> +			return -EINVAL;
> +		}
> +
> +		erase_op.mtd = mtd;
> +		erase_op.addr = off;
> +		erase_op.len = len;
> +		erase_op.scrub = scrub;
> +
> +		ret = mtd_erase(mtd, &erase_op);
> +	} else {
> +		return CMD_RET_USAGE;
> +	}
> +
> +	return ret;
> +}
> +
> +static char mtd_help_text[] =
> +#ifdef CONFIG_SYS_LONGHELP
> +	"- generic operations on memory technology devices\n\n"
> +	"mtd list\n"
> +	"mtd read[.raw][.oob]           <name> <addr> [<off> [<size>]]\n"
> +	"mtd dump[.raw][.oob]           <name>        [<off> [<size>]]\n"
> +	"mtd write[.raw][.oob]          <name> <addr> [<off> [<size>]]\n"
> +	"mtd erase[.chip][.dontskipbad] <name>        [<off> [<size>]]\n"
> +	"\n"
> +	"With:\n"
> +	"\t<name>: NAND partition/chip name\n"
> +	"\t<addr>: user address from/to which data will be retrieved/stored\n"
> +	"\t<off>: offset in <name> in bytes (default: start of the part)\n"
> +	"\t\t* must be block-aligned for erase\n"
> +	"\t\t* must be page-aligned otherwise\n"
> +	"\t<size>: length of the operation in bytes\n"
> +	"\t\t* must be a multiple of a block for erase (default: 1 block)\n"
> +	"\t\t* must be a multiple of a page otherwise (default: 1 page)\n"
> +#endif
> +	"";
> +
> +U_BOOT_CMD(mtd, 10, 1, do_mtd, "MTD utils", mtd_help_text);