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

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