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

[Boris Brezillon <boris.brezillon@bootlin.com>]

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.

> +
> +	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/ ? And why not passing an mtd_oob_ops
object directly? That would reduce the number of parameters you pass to
this function.

> +{
> +	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.

> +	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. 

> +
> +		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.

> +	}
> +
> +	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?

> +	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.

> +
> +	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.

> +
> +		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)?

> +			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.

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