From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Date: Wed, 11 Jul 2018 15:51:39 +0200 Subject: [U-Boot] [RFC PATCH 17/20] cmd: mtd: add 'mtd' command In-Reply-To: <20180606214524.550442e2@bbrezillon> References: <20180606153040.21397-1-miquel.raynal@bootlin.com> <20180606153040.21397-18-miquel.raynal@bootlin.com> <20180606214524.550442e2@bbrezillon> Message-ID: <20180711155139.69f381e2@xps13> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de Hi Boris, Boris Brezillon wrote on Wed, 6 Jun 2018 21:45:24 +0200: > Hi Miquel, > > On Wed, 6 Jun 2018 17:30:37 +0200 > Miquel Raynal wrote: > > > 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. > > Just a few comments below, but overall, I'm really happy with this new > set of commands and the fact that we'll soon be able to replace custom > MTD accessors (nand, onenand, sf, cp.b+erase, ...) by these ones. > > > > > Signed-off-by: Miquel Raynal > > --- > > cmd/Kconfig | 5 + > > cmd/Makefile | 1 + > > cmd/mtd.c | 280 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/mtd/Makefile | 2 +- > > 4 files changed, 287 insertions(+), 1 deletion(-) > > create mode 100644 cmd/mtd.c > > > > 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..fe48378bd0 > > --- /dev/null > > +++ b/cmd/mtd.c > > @@ -0,0 +1,280 @@ > > +// 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 > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +static void mtd_dump_buf(u8 *buf, uint len) > > +{ > > + int i, j; > > + > > + for (i = 0; i < len; ) { > > + printf("0x%08x:\t", 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 int do_mtd_list(void) > > +{ > > + struct mtd_info *mtd; > > + struct udevice *dev; > > + int dm_idx = 0, idx = 0; > > + > > + /* Ensure all devices compliants with U-Boot driver model are probed */ > > + while (!uclass_find_device(UCLASS_MTD, dm_idx, &dev) && dev) { > > + mtd_probe(dev); > > + dm_idx++; > > + } > > + > > + printf("MTD devices list (%d DM compliant):\n", dm_idx); > > Do we really want to say how many of them are exported by DM compliant > drivers? I mean, the user doesn't care about that. If you want to force > people to convert their drivers, we should probably complain at MTD > device registration time when the mtd_info struct is not backed by an > udevice. It was more like a small debug value but I don't really care about it. Parenthesis removed. > > > + > > + mtd_for_each_device(mtd) { > > + mtd_show_device(mtd); > > + idx++; > > + } > > + > > + if (!idx) > > + printf("No MTD device found\n"); > > + > > + return 0; > > +} > > + > > +static int do_mtd_read_write(struct mtd_info *mtd, bool read, uint *waddr, > > + bool raw, bool woob, u64 from, u64 len) > > s/do_mtd_read_write/do_mtd_io/ ? +1 for the artistic name :) -> renamed > And why not passing an mtd_oob_ops > object directly? That would reduce the number of parameters you pass to > this function. Good point actually. While rewriting the function I figured out there was no "reusable code" nor any "logicial split" with this do_mtd_io() helper so I just moved the code from it into the main do_mtd(). If you find it unclear please tell me where > > > +{ > > + u32 buf_len = woob ? mtd->writesize + mtd->oobsize : > > + ROUND(len, mtd->writesize); > > + u8 *buf = malloc(buf_len); > > It's probably worth a comment explaining why you allocate a bounce > buffer here (i.e. to make sure len not aligned on a page size are padded > with 0xff). > > Maybe a simpler solution would be to simply refuse such unaligned > accesses. Agreed. > > > + struct mtd_oob_ops ops = { > > + .mode = raw ? MTD_OPS_RAW : 0, > > + .len = len, > > + .ooblen = woob ? mtd->oobsize : 0, > > + .datbuf = buf, > > + .oobbuf = woob ? &buf[mtd->writesize] : NULL, > > + }; > > + int ret; > > + > > + if (!buf) > > + return -ENOMEM; > > + > > + memset(buf, 0xFF, buf_len); > > + > > + if (read) { > > + ret = mtd_read_oob(mtd, from, &ops); > > + } else { > > + memcpy(buf, waddr, ops.len + ops.ooblen); > > + ret = mtd_write_oob(mtd, from, &ops); > > + } > > + > > + if (ret) { > > + printf("Could not handle %lldB from 0x%08llx on %s, ret %d\n", > > + len, from, mtd->name, ret); > > + return ret; > > + } > > + > > + if (read) { > > + printf("Dump %lld data bytes from 0x%08llx:\n", len, from); > > + mtd_dump_buf(buf, len); > > Read and dump are 2 different things: one might want to read an MTD > device and store the result in RAM without dumping it on the console. Sure. Made a difference between read and dump. > > > + > > + if (woob) { > > + printf("\nDump %d OOB bytes from 0x%08llx:\n", > > + mtd->oobsize, from); > > + mtd_dump_buf(&buf[len], mtd->oobsize); > > + } > > Looks like you're never copying the data back to waddr. Fixed as there is no more bounce buffer. > > > + } > > + > > + kfree(buf); > > + > > + return 0; > > +} > > + > > +static int do_mtd_erase(struct mtd_info *mtd, bool scrub, u64 from, u64 len) > > +{ > > + struct erase_info erase_infos = { > > + .mtd = mtd, > > + .addr = from, > > + .len = len, > > + .scrub = scrub, > > + }; > > + > > + return mtd_erase(mtd, &erase_infos); > > +} > > + > > +static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > +{ > > + struct mtd_info *mtd; > > + struct udevice *dev; > > + const char *cmd; > > + char *part; > > + 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; > > + > > + part = argv[2]; > > Why part. The MTD object can be a partition or the device itself. How > about renaming it mtdname? Ok. > > > + ret = uclass_find_device_by_name(UCLASS_MTD, part, &dev); > > + if (!ret && dev) { > > + mtd_probe(dev); > > + mtd = (struct mtd_info *)dev_get_uclass_priv(dev); > > + if (!mtd) { > > + printf("Could not retrieve MTD data\n"); > > + return -ENODEV; > > + } > > + } else { > > + mtd = get_mtd_device_nm(part); > > + if (IS_ERR_OR_NULL(mtd)) { > > + printf("MTD device %s not found, ret %ld\n", part, > > + PTR_ERR(mtd)); > > + return 1; > > + } > > + } > > Hm, I'd do it the other way around: first call get_mtd_device_nm() and > if you don't find the device trigger the probe of all UCLASS_MTD devs, > and then search again with get_mtd_device_nm(). Note that > mtd->dev->name and mtd->name are 2 different things, and they won't > match most of the time. Actually the logic above was broken in the sense that an 'mtd list' was necessary prior to using any DM-compliant driven device. Edited. > > > + > > + argc -= 3; > > + argv += 3; > > + > > + /* Do the parsing */ > > + if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "write", 5)) { > > + bool read, raw, woob; > > + uint *waddr = NULL; > > + u64 off, len; > > + > > + read = !strncmp(cmd, "read", 4); > > + raw = strstr(cmd, ".raw"); > > + woob = strstr(cmd, ".oob"); > > + > > + if (!read) { > > + if (argc < 1) > > + return CMD_RET_USAGE; > > + > > + waddr = map_sysmem(simple_strtoul(argv[0], NULL, 10), > > + 0); > > + argc--; > > + argv++; > > + } > > + > > + off = argc > 0 ? simple_strtoul(argv[0], NULL, 10) : 0; > > + len = argc > 1 ? simple_strtoul(argv[1], NULL, 10) : > > + mtd->writesize + (woob ? mtd->oobsize : 0); > > + > > + if ((u32)off % mtd->writesize) { > > + printf("Section not page-aligned (0x%x)\n", > > + mtd->writesize); > > + return -EINVAL; > > + } > > + > > + if (woob && (len != (mtd->writesize + mtd->oobsize))) { > > + printf("OOB operations are limited to single pages\n"); > > + return -EINVAL; > > + } > > Is this a uboot limitation? I don't think you have such a limitation in > Linux. Kind of, only one single page write with OOB at a time is possible says a comment on mtd_oob_ops in mtd.h in Linux. Reads are actually not limited. But I really prefer to keep this limitation that simplifies _a lot_ the logic and is not really useful to a u-boot user I suppose. > > > + > > + if ((off + len) >= mtd->size) { > > That doesn't work when reading the last page of the MTD device with > woob = true. See how Linux handle that here [1]. BTW, why don't you let > mtdcore.c do these checks for you (that's also true for unaligned > accesses)? Because the relevant patch (and its fix :) ) has not been backported yet. And now I understand your voice in my ears "do it". Okay. > > > + printf("Access location beyond the end of the chip\n"); > > + return -EINVAL; > > + } > > + > > + printf("%s (from %p) %lldB at 0x%08llx [%s %s]\n", > > + read ? "read" : "write", read ? 0 : waddr, len, off, > > + raw ? "raw" : "", woob ? "oob" : ""); > > + > > + ret = do_mtd_read_write(mtd, read, waddr, raw, woob, off, len); > > + > > + if (!read) > > + unmap_sysmem(waddr); > > + > > + } else if (!strcmp(cmd, "erase") || !strcmp(cmd, "scrub")) { > > + bool scrub = !strcmp(cmd, "scrub"); > > + bool full_erase = !strncmp(&cmd[5], ".chip", 4); > > + u64 off, len; > > + > > + off = argc > 0 ? simple_strtoul(argv[0], NULL, 10) : 0; > > + len = argc > 1 ? simple_strtoul(argv[1], NULL, 10) : > > + mtd->erasesize; > > + if (full_erase) { > > + off = 0; > > + len = mtd->size; > > + } > > + > > + if ((u32)off % mtd->erasesize) { > > + printf("Section not erase-block-aligned (0x%x)\n", > > + mtd->erasesize); > > + return -EINVAL; > > + } > > + > > + if ((u32)len % mtd->erasesize) { > > + printf("Size not aligned with an erase block (%dB)\n", > > + mtd->erasesize); > > + return -EINVAL; > > + } > > + > > + if ((off + len) >= mtd->size) { > > + printf("Cannot read beyond end of chip\n"); > > + return -EINVAL; > > + } > > + > > + ret = do_mtd_erase(mtd, scrub, off, len); > > + } 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] [ []]\n" > > I guess this one should be > > "mtd read[.raw][.oob] [ []]\n" > > and then, you should have > > "mtd dump[.raw][.oob] [ []]\n" > > > + "mtd write[.raw][.oob] [ []]\n" > > + "mtd erase[.chip] [ []]\n" > > + "mtd scrub[.chip] [ []]\n" > > Hm, maybe it's time to simplify that. mtd.scrub is just an option of mtd > erase, so maybe we should just have: > > mtd erase[.force] or erase[.dontskipbad] > > Also, [.chip] can be extracted from the number of parameters. If you > just have passed, that means the callers wants to erase the > whole chip. I prefer .dontskipbad for the sake of clarity. Updated accordingly. Also updated the code following Stefan comments: all the numbers (addresses, lengths) are hexadecimal + creation of a dump command. Thanks for the review! Miquèl > > Regards, > > Boris > > > +#endif > > + ""; > > + > > +U_BOOT_CMD(mtd, 10, 1, do_mtd, "MTD utils", mtd_help_text); > > [1]https://elixir.bootlin.com/linux/latest/source/drivers/mtd/mtdcore.c#L1117 -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com