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

On Wed, 11 Jul 2018 17:25:28 +0200 Miquel Raynal miquel.raynal@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);