[U-Boot] [RFC PATCH 17/20] cmd: mtd: add 'mtd' command

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

Boris Brezillon <boris.brezillon@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@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@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@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