Message ID | 20180711152529.24547-21-miquel.raynal@bootlin.com |
---|---|
State | Superseded |
Delegated to: | Jagannadha Sutradharudu Teki |
Headers | show |
Series | SPI-NAND support | expand |
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);
Hi Boris, Boris Brezillon <boris.brezillon@bootlin.com> wrote on Thu, 12 Jul 2018 00:42:13 +0200: > 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? Absolutely. > > > + if (mtd->dev) > > + printf(" [device: %s] [parent: %s] [driver: %s]", > > + mtd->dev->name, mtd->dev->parent->name, > > + mtd->dev->driver->name); > > + > > + printf("\n"); > > +} [...] > > + } 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. ".chip" removed. Default now is mtd->size (the entire MTD device) for erase/read/write, and mtd->writesize (a page) for a dump. > > > + 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. See above. > > > + > > + if (full_erase) { > > + off = 0; > > + len = mtd->size; > > + } > > + > > + if ((u32)off % mtd->erasesize) { > > Sounds dangerous. We have 8GB NANDs on sunxi platforms... [...] > > + > > + if ((u32)len % mtd->erasesize) { > > Same here. I guess there's a do_div() in uboot. In both cases I take the less significant bytes of a 64-bit value. The modulo operation is safe as long as mtd->erasesize is a 32-bit value too. I don't think there is any danger? [...] Thanks, Miquèl
Hi Boris, > > > + > > > + if (full_erase) { > > > + off = 0; > > > + len = mtd->size; > > > + } > > > + > > > + if ((u32)off % mtd->erasesize) { > > > > Sounds dangerous. We have 8GB NANDs on sunxi platforms... > > [...] > > > > + > > > + if ((u32)len % mtd->erasesize) { > > > > Same here. I guess there's a do_div() in uboot. > > In both cases I take the less significant bytes of a 64-bit value. The > modulo operation is safe as long as mtd->erasesize is a 32-bit value > too. I don't think there is any danger? As discussed out of this thread, this works because mtd->erasesize is always a power of 2. Erase sizes not being a power of 2 do not exist but it will be safer to switch to do_div(). Thanks, Miquèl
diff --git a/cmd/Kconfig b/cmd/Kconfig index 136836d146..6e9b629e1c 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -797,6 +797,11 @@ config CMD_MMC help MMC memory mapped support. +config CMD_MTD + bool "mtd" + help + MTD commands support. + config CMD_NAND bool "nand" default y if NAND_SUNXI diff --git a/cmd/Makefile b/cmd/Makefile index 9a358e4801..e42db12e1d 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -88,6 +88,7 @@ obj-$(CONFIG_CMD_MISC) += misc.o obj-$(CONFIG_CMD_MMC) += mmc.o obj-$(CONFIG_CMD_MMC_SPI) += mmc_spi.o obj-$(CONFIG_MP) += mp.o +obj-$(CONFIG_CMD_MTD) += mtd.o obj-$(CONFIG_CMD_MTDPARTS) += mtdparts.o obj-$(CONFIG_CMD_NAND) += nand.o obj-$(CONFIG_CMD_NET) += net.o diff --git a/cmd/mtd.c b/cmd/mtd.c new file mode 100644 index 0000000000..c0809e9ed1 --- /dev/null +++ b/cmd/mtd.c @@ -0,0 +1,287 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * mtd.c + * + * Generic command to handle basic operations on any memory device. + * + * Copyright: Bootlin, 2018 + * Author: Miquèl Raynal <miquel.raynal@bootlin.com> + */ + +#include <command.h> +#include <common.h> +#include <console.h> +#include <linux/mtd/mtd.h> +#include <malloc.h> +#include <mapmem.h> +#include <mtd.h> +#include <dm/device.h> +#include <dm/uclass-internal.h> + +static void mtd_dump_buf(u8 *buf, uint len, uint offset) +{ + int i, j; + + for (i = 0; i < len; ) { + printf("0x%08x:\t", offset + i); + for (j = 0; j < 8; j++) + printf("%02x ", buf[i + j]); + printf(" "); + i += 8; + for (j = 0; j < 8; j++) + printf("%02x ", buf[i + j]); + printf("\n"); + i += 8; + } +} + +static void mtd_show_device(struct mtd_info *mtd) +{ + printf("* %s", mtd->name); + if (mtd->dev) + printf(" [device: %s] [parent: %s] [driver: %s]", + mtd->dev->name, mtd->dev->parent->name, + mtd->dev->driver->name); + + printf("\n"); +} + +static void mtd_probe_devices_dm(void) +{ + struct udevice *dev; + int idx = 0; + + /* Devices with DM compliant drivers */ + while (!uclass_find_device(UCLASS_MTD, idx, &dev) && dev) { + mtd_probe(dev); + idx++; + } +} + +static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len) +{ + do_div(len, mtd->writesize); + + return len; +} + +static int do_mtd_list(void) +{ + struct mtd_info *mtd; + int dev_nb = 0; + + /* Ensure all devices are probed */ + mtd_probe_devices_dm(); + + printf("List of MTD devices:\n"); + mtd_for_each_device(mtd) { + mtd_show_device(mtd); + dev_nb++; + } + + if (!dev_nb) { + printf("No MTD device found\n"); + return -EINVAL; + } + + return 0; +} + +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"); + 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; + + if (full_erase) { + off = 0; + len = mtd->size; + } + + if ((u32)off % mtd->erasesize) { + printf("Offset not aligned with a block (0x%x)\n", + mtd->erasesize); + return -EINVAL; + } + + if ((u32)len % mtd->erasesize) { + 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); diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile index eb0e7264ab..95b5f25fef 100644 --- a/drivers/mtd/Makefile +++ b/drivers/mtd/Makefile @@ -5,7 +5,7 @@ # SPDX-License-Identifier: GPL-2.0+ # -ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND)$(CONFIG_CMD_SF))) +ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND)$(CONFIG_CMD_SF)$(CONFIG_CMD_MTD))) obj-y += mtdcore.o mtd_uboot.o endif obj-$(CONFIG_MTD) += mtd-uclass.o
There should not be a 'nand' command, a 'sf' command and certainly not another 'spi-nand'. Write a 'mtd' command instead to manage all MTD devices at once. This should be the preferred way to access any MTD device. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- cmd/Kconfig | 5 + cmd/Makefile | 1 + cmd/mtd.c | 287 +++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/mtd/Makefile | 2 +- 4 files changed, 294 insertions(+), 1 deletion(-) create mode 100644 cmd/mtd.c