[U-Boot] [RFC PATCH 17/20] cmd: mtd: add 'mtd' command
Miquel Raynal
miquel.raynal at bootlin.com
Wed Jul 11 13:51:39 UTC 2018
Hi Boris,
Boris Brezillon <boris.brezillon at bootlin.com> wrote on Wed, 6 Jun 2018
21:45:24 +0200:
> Hi Miquel,
>
> On Wed, 6 Jun 2018 17:30:37 +0200
> Miquel Raynal <miquel.raynal at bootlin.com> 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 <miquel.raynal at bootlin.com>
> > ---
> > 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 <miquel.raynal at bootlin.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <linux/mtd/mtd.h>
> > +#include <command.h>
> > +#include <console.h>
> > +#include <malloc.h>
> > +#include <mtd.h>
> > +#include <mapmem.h>
> > +#include <dm/device.h>
> > +#include <dm/uclass-internal.h>
> > +
> > +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] <name> [<off> [<size>]]\n"
>
> I guess this one should be
>
> "mtd read[.raw][.oob] <name> <addr> [<off> [<size>]]\n"
>
> and then, you should have
>
> "mtd dump[.raw][.oob] <name> [<off> [<size>]]\n"
>
> > + "mtd write[.raw][.oob] <name> <addr> [<off> [<size>]]\n"
> > + "mtd erase[.chip] <name> [<off> [<size>]]\n"
> > + "mtd scrub[.chip] <name> [<off> [<size>]]\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 <name> 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
More information about the U-Boot
mailing list