[U-Boot] [PATCH v7 00/13] Cleaner MTD devices management

[U-Boot] [PATCH v7 00/13] Cleaner MTD devices management

[Miquel Raynal]

The introduction of the SPI-NAND framework and, more widely, the
support of non parallel-NOR, parallel-NAND and SPI-NOR has shown that
extending U-Boot MTD devices/partitions handling was not
straightforward at all. While this first series [1] is a great step
forward, it is not actually usable due to purely software
limitations.

This series intends to:
* Do some cleanup in the MTD layer
* Add a new 'mtd' command that should replace in the future all the
  MTD-specific commands like 'nand' and 'sf'.
* Make deep changes in the partition handling by introducing a new way
  of managing MTD partitions (that soon will be also sent to the Linux
  kernel), and adding a generic 'mtdparts' environment variable parser
  in prevision of the 'mtdparts' command removal.
* The UBI command is also updated to stop messing with partitions that
  should just be declared in the 'mtdparts' variable.

People can now do the following without 'mtdids':
# setenv mtdparts "nor0=1m(some),-(more);spi-nand0=-(all)"

Partitions are visible with:
# mtd list

And UBI support is available with (for the above example):
# ubi part all

At least these two missing features will be added with further changes:
* 'mtd write' on NANDs should use by default the trimffs option (to
  avoid writing empty pages).
* 'nand bad' is a great way to know the list of detected bad-blocks
  and could be implemented in 'mtd'.

Of course, this series only applies on top of [1].

[1] https://lists.denx.de/pipermail/u-boot/2018-August/336780.html


Thanks,
Miquèl


Miquel Raynal (13):
  lib: strto: parse all lowercase metric prefixes in ustrtoul[l]
  lib: strto: fix metric suffix parsing in strtoul[l]
  mtd: Kconfig: remove MTD_PARTITIONS duplicated symbol
  cmd: mtdparts: accept spi-nand devices
  cmd: mtdparts: remove mandatory 'mtdparts=' prefix
  mtd: uclass: add probe function
  mtd: uclass: add a generic 'mtdparts' parser
  mtd: uclass: search for an equivalent MTD name with the mtdids
  mtd: mtdpart: implement proper partition handling
  cmd: mtd: add 'mtd' command
  cmd: mtdparts: try to probe the MTD devices as a fallback
  cmd: ubi: clean the partition handling
  cmd: mtdparts: describe as legacy

 cmd/Kconfig                    |  22 +-
 cmd/Makefile                   |   1 +
 cmd/mtd.c                      | 517 ++++++++++++++++++++++++++
 cmd/mtdparts.c                 |  46 ++-
 cmd/ubi.c                      | 100 ++---
 drivers/mtd/Kconfig            |   3 -
 drivers/mtd/Makefile           |   2 +-
 drivers/mtd/mtd-uclass.c       |  16 +
 drivers/mtd/mtdcore.c          |   2 +
 drivers/mtd/mtdpart.c          | 661 +++++++++++++++++++++------------
 include/jffs2/load_kernel.h    |   7 +-
 include/linux/mtd/mtd.h        |  31 ++
 include/linux/mtd/partitions.h |   5 +-
 include/mtd.h                  |   3 +
 lib/strto.c                    |  26 +-
 15 files changed, 1093 insertions(+), 349 deletions(-)
 create mode 100644 cmd/mtd.c

-- 
2.17.1

[U-Boot] [PATCH v7 01/13] lib: strto: parse all lowercase metric prefixes in ustrtoul[l]

[Miquel Raynal]

Both ustrtoul and ustrtoull interpret 1k but not 1m or 1g. Even if the
SI symbols for Mega and Giga are 'M' and 'G', certain entries of
eg. mtdparts also use (wrongly) the metric prefix 'm' and 'g'.

I do not see how parsing lowercase prefixes could break anything, so
parse them like their uppercase counterpart.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 lib/strto.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/strto.c b/lib/strto.c
index 7f6076909a..84f8d92d57 100644
--- a/lib/strto.c
+++ b/lib/strto.c
@@ -87,9 +87,11 @@ unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
 	unsigned long result = simple_strtoul(cp, endp, base);
 	switch (**endp) {
 	case 'G':
+	case 'g':
 		result *= 1024;
 		/* fall through */
 	case 'M':
+	case 'm':
 		result *= 1024;
 		/* fall through */
 	case 'K':
@@ -110,9 +112,11 @@ unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
 	unsigned long long result = simple_strtoull(cp, endp, base);
 	switch (**endp) {
 	case 'G':
+	case 'g':
 		result *= 1024;
 		/* fall through */
 	case 'M':
+	case 'm':
 		result *= 1024;
 		/* fall through */
 	case 'K':
-- 
2.17.1

[U-Boot] [PATCH v7 02/13] lib: strto: fix metric suffix parsing in strtoul[l]

[Miquel Raynal]

While 1kB or 1kiB will be parsed correctly, 1k will return the right
amount, but the metric suffix will not be escaped once the char
pointer updated. Fix this situation by simplifying the move of the
endp pointer.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 lib/strto.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/lib/strto.c b/lib/strto.c
index 84f8d92d57..502a0153e7 100644
--- a/lib/strto.c
+++ b/lib/strto.c
@@ -97,12 +97,11 @@ unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
 	case 'K':
 	case 'k':
 		result *= 1024;
-		if ((*endp)[1] == 'i') {
-			if ((*endp)[2] == 'B')
-				(*endp) += 3;
-			else
-				(*endp) += 2;
-		}
+		(*endp)++;
+		if (**endp == 'i')
+			(*endp)++;
+		if (**endp == 'B')
+			(*endp)++;
 	}
 	return result;
 }
@@ -122,12 +121,11 @@ unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
 	case 'K':
 	case 'k':
 		result *= 1024;
-		if ((*endp)[1] == 'i') {
-			if ((*endp)[2] == 'B')
-				(*endp) += 3;
-			else
-				(*endp) += 2;
-		}
+		(*endp)++;
+		if (**endp == 'i')
+			(*endp)++;
+		if (**endp == 'B')
+			(*endp)++;
 	}
 	return result;
 }
-- 
2.17.1

[U-Boot] [PATCH v7 03/13] mtd: Kconfig: remove MTD_PARTITIONS duplicated symbol

[Miquel Raynal]

MTD_PARTITIONS is declared twice. Remove the redundant entry with no
help associated.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/Kconfig | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index d98457e223..9341d518f3 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -1,8 +1,5 @@
 menu "MTD Support"
 
-config MTD_PARTITIONS
-	bool
-
 config MTD
 	bool "Enable Driver Model for MTD drivers"
 	depends on DM
-- 
2.17.1

[U-Boot] [PATCH v7 04/13] cmd: mtdparts: accept spi-nand devices

[Miquel Raynal]

Let spi-nand devices be recognized by mtdparts. This is superfluous
but a full mtdparts rework would be very time-consuming.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jagan Teki <jagan@openedev.com>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 cmd/mtdparts.c              | 13 ++++++++-----
 include/jffs2/load_kernel.h |  7 +++++--
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 756fc6018f..2e547894c6 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -37,7 +37,7 @@
  * mtdids=<idmap>[,<idmap>,...]
  *
  * <idmap>    := <dev-id>=<mtd-id>
- * <dev-id>   := 'nand'|'nor'|'onenand'<dev-num>
+ * <dev-id>   := 'nand'|'nor'|'onenand'|'spi-nand'<dev-num>
  * <dev-num>  := mtd device number, 0...
  * <mtd-id>   := unique device tag used by linux kernel to find mtd device (mtd->name)
  *
@@ -339,7 +339,7 @@ static int part_validate_eraseblock(struct mtdids *id, struct part_info *part)
 
 	if (!mtd->numeraseregions) {
 		/*
-		 * Only one eraseregion (NAND, OneNAND or uniform NOR),
+		 * Only one eraseregion (NAND, SPI-NAND, OneNAND or uniform NOR),
 		 * checking for alignment is easy here
 		 */
 		offset = part->offset;
@@ -1030,7 +1030,7 @@ static struct mtdids* id_find_by_mtd_id(const char *mtd_id, unsigned int mtd_id_
 }
 
 /**
- * Parse device id string <dev-id> := 'nand'|'nor'|'onenand'<dev-num>,
+ * Parse device id string <dev-id> := 'nand'|'nor'|'onenand'|'spi-nand'<dev-num>,
  * return device type and number.
  *
  * @param id string describing device id
@@ -1054,6 +1054,9 @@ int mtd_id_parse(const char *id, const char **ret_id, u8 *dev_type,
 	} else if (strncmp(p, "onenand", 7) == 0) {
 		*dev_type = MTD_DEV_TYPE_ONENAND;
 		p += 7;
+	} else if (strncmp(p, "spi-nand", 8) == 0) {
+		*dev_type = MTD_DEV_TYPE_SPINAND;
+		p += 8;
 	} else {
 		printf("incorrect device type in %s\n", id);
 		return 1;
@@ -1636,7 +1639,7 @@ static int parse_mtdids(const char *const ids)
 	while(p && (*p != '\0')) {
 
 		ret = 1;
-		/* parse 'nor'|'nand'|'onenand'<dev-num> */
+		/* parse 'nor'|'nand'|'onenand'|'spi-nand'<dev-num> */
 		if (mtd_id_parse(p, &p, &type, &num) != 0)
 			break;
 
@@ -2112,7 +2115,7 @@ static char mtdparts_help_text[] =
 	"'mtdids' - linux kernel mtd device id <-> u-boot device id mapping\n\n"
 	"mtdids=<idmap>[,<idmap>,...]\n\n"
 	"<idmap>    := <dev-id>=<mtd-id>\n"
-	"<dev-id>   := 'nand'|'nor'|'onenand'<dev-num>\n"
+	"<dev-id>   := 'nand'|'nor'|'onenand'|'spi-nand'<dev-num>\n"
 	"<dev-num>  := mtd device number, 0...\n"
 	"<mtd-id>   := unique device tag used by linux kernel to find mtd device (mtd->name)\n\n"
 	"'mtdparts' - partition list\n\n"
diff --git a/include/jffs2/load_kernel.h b/include/jffs2/load_kernel.h
index 1ddff062ad..9346d7ee9f 100644
--- a/include/jffs2/load_kernel.h
+++ b/include/jffs2/load_kernel.h
@@ -15,9 +15,12 @@
 #define MTD_DEV_TYPE_NOR	0x0001
 #define MTD_DEV_TYPE_NAND	0x0002
 #define MTD_DEV_TYPE_ONENAND	0x0004
+#define MTD_DEV_TYPE_SPINAND	0x0008
 
-#define MTD_DEV_TYPE(type) ((type == MTD_DEV_TYPE_NAND) ? "nand" :	\
-			(type == MTD_DEV_TYPE_ONENAND) ? "onenand" : "nor")
+#define MTD_DEV_TYPE(type) (type == MTD_DEV_TYPE_NAND ? "nand" :	\
+			    (type == MTD_DEV_TYPE_NOR ? "nor" :		\
+			     (type == MTD_DEV_TYPE_ONENAND ? "onenand" : \
+			      "spi-nand")))				\
 
 struct mtd_device {
 	struct list_head link;
-- 
2.17.1

[U-Boot] [PATCH v7 05/13] cmd: mtdparts: remove mandatory 'mtdparts=' prefix

[Miquel Raynal]

All U-Boot users must define the mtdparts environment variable with:
setenv mtdparts mtdparts=...

While this may ease the partition declaration job to be passed to
Linux, this is a pure software limitation and forcing this prefix is a
complete non-sense. Let the user to declare manually the mtdparts
variable without the prefix.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jagan Teki <jagan@openedev.com>
---
 cmd/mtdparts.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 2e547894c6..f7ed1a0779 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -44,7 +44,7 @@
  *
  * 'mtdparts' - partition list
  *
- * mtdparts=mtdparts=<mtd-def>[;<mtd-def>...]
+ * mtdparts=[mtdparts=]<mtd-def>[;<mtd-def>...]
  *
  * <mtd-def>  := <mtd-id>:<part-def>[,<part-def>...]
  * <mtd-id>   := unique device tag used by linux kernel to find mtd device (mtd->name)
@@ -62,11 +62,11 @@
  *
  * 1 NOR Flash, with 1 single writable partition:
  * mtdids=nor0=edb7312-nor
- * mtdparts=mtdparts=edb7312-nor:-
+ * mtdparts=[mtdparts=]edb7312-nor:-
  *
  * 1 NOR Flash with 2 partitions, 1 NAND with one
  * mtdids=nor0=edb7312-nor,nand0=edb7312-nand
- * mtdparts=mtdparts=edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
+ * mtdparts=[mtdparts=]edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
  *
  */
 
@@ -1099,9 +1099,6 @@ static int generate_mtdparts(char *buf, u32 buflen)
 		return 0;
 	}
 
-	strcpy(p, "mtdparts=");
-	p += 9;
-
 	list_for_each(dentry, &devices) {
 		dev = list_entry(dentry, struct mtd_device, link);
 
@@ -1572,11 +1569,9 @@ static int parse_mtdparts(const char *const mtdparts)
 	if (!p)
 		p = mtdparts;
 
-	if (strncmp(p, "mtdparts=", 9) != 0) {
-		printf("mtdparts variable doesn't start with 'mtdparts='\n");
-		return err;
-	}
-	p += 9;
+	/* Skip the useless prefix, if any */
+	if (strncmp(p, "mtdparts=", 9) == 0)
+		p += 9;
 
 	while (*p != '\0') {
 		err = 1;
-- 
2.17.1

[U-Boot] [PATCH v7 06/13] mtd: uclass: add probe function

[Miquel Raynal]

The user might want to trigger the probe of any MTD device, export these
functions so they can be called from a command source file.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jagan Teki <jagan@openedev.com>
---
 drivers/mtd/mtd-uclass.c | 16 ++++++++++++++++
 include/mtd.h            |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/mtd/mtd-uclass.c b/drivers/mtd/mtd-uclass.c
index 9ca049c437..5418217431 100644
--- a/drivers/mtd/mtd-uclass.c
+++ b/drivers/mtd/mtd-uclass.c
@@ -5,9 +5,25 @@
 
 #include <common.h>
 #include <dm.h>
+#include <dm/device-internal.h>
 #include <errno.h>
 #include <mtd.h>
 
+/**
+ * mtd_probe - Probe the device @dev if not already done
+ *
+ * @dev: U-Boot device to probe
+ *
+ * @return 0 on success, an error otherwise.
+ */
+int mtd_probe(struct udevice *dev)
+{
+	if (device_active(dev))
+		return 0;
+
+	return device_probe(dev);
+}
+
 /*
  * Implement a MTD uclass which should include most flash drivers.
  * The uclass private is pointed to mtd_info.
diff --git a/include/mtd.h b/include/mtd.h
index 548e7f191b..6e6da3002f 100644
--- a/include/mtd.h
+++ b/include/mtd.h
@@ -19,4 +19,6 @@ static inline struct mtd_info *mtd_get_info(struct udevice *dev)
 	return dev_get_uclass_priv(dev);
 }
 
+int mtd_probe(struct udevice *dev);
+
 #endif	/* _MTD_H_ */
-- 
2.17.1

[U-Boot] [PATCH v7 07/13] mtd: uclass: add a generic 'mtdparts' parser

[Miquel Raynal]

The current parser is very specific to U-Boot mtdparts implementation.
It does not use MTD structures like mtd_info and mtd_partition. Copy
and adapt the current parser in drivers/mtd/mtd-uclass.c (to not break
the current use of mtdparts.c itself) and write some kind of a wrapper
around the current implementation to allow other commands to benefit
from this parsing in a user-friendly way.

This new function will allocate an mtd_partition array for each
successful call. This array must be freed after use by the caller.
The given 'mtdparts' buffer pointer will be moved forward to the next
MTD device (if any, it will point towards a '\0' character otherwise).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jagan Teki <jagan@openedev.com>
---
 drivers/mtd/mtdpart.c          | 187 +++++++++++++++++++++++++++++++++
 include/linux/mtd/partitions.h |   2 +
 2 files changed, 189 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 9ccb1b3361..d9708da2ae 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -76,6 +76,193 @@ char *kstrdup(const char *s, gfp_t gfp)
 }
 #endif
 
+#define MTD_SIZE_REMAINING		(~0LLU)
+#define MTD_OFFSET_NOT_SPECIFIED	(~0LLU)
+
+/**
+ * mtd_parse_partition - Parse @mtdparts partition definition, fill @partition
+ *                       with it and update the @mtdparts string pointer.
+ *
+ * The partition name is allocated and must be freed by the caller.
+ *
+ * This function is widely inspired from part_parse (mtdparts.c).
+ *
+ * @mtdparts: String describing the partition with mtdparts command syntax
+ * @partition: MTD partition structure to fill
+ *
+ * @return 0 on success, an error otherwise.
+ */
+static int mtd_parse_partition(const char **_mtdparts,
+			       struct mtd_partition *partition)
+{
+	const char *mtdparts = *_mtdparts;
+	const char *name = NULL;
+	int name_len;
+	char *buf;
+
+	/* Ensure the partition structure is empty */
+	memset(partition, 0, sizeof(struct mtd_partition));
+
+	/* Fetch the partition size */
+	if (*mtdparts == '-') {
+		/* Assign all remaining space to this partition */
+		partition->size = MTD_SIZE_REMAINING;
+		mtdparts++;
+	} else {
+		partition->size = ustrtoull(mtdparts, (char **)&mtdparts, 0);
+		if (partition->size < SZ_4K) {
+			printf("Minimum allowed partition size is 4kiB\n");
+			return -EINVAL;
+		}
+	}
+
+	/* Check for the offset */
+	partition->offset = MTD_OFFSET_NOT_SPECIFIED;
+	if (*mtdparts == '@') {
+		mtdparts++;
+		partition->offset = ustrtoull(mtdparts, (char **)&mtdparts, 0);
+	}
+
+	/* Now look for the name */
+	if (*mtdparts == '(') {
+		name = ++mtdparts;
+		if ((mtdparts = strchr(name, ')')) == NULL) {
+			printf("No closing ')' found in partition name\n");
+			return -EINVAL;
+		}
+		name_len = mtdparts - name + 1;
+		if ((name_len - 1) == 0) {
+			printf("Empty partition name\n");
+			return -EINVAL;
+		}
+		mtdparts++;
+	} else {
+		/* Name will be of the form size at offset */
+		name_len = 22;
+	}
+
+	/* Check if the partition is read-only */
+	if (strncmp(mtdparts, "ro", 2) == 0) {
+		partition->mask_flags |= MTD_WRITEABLE;
+		mtdparts += 2;
+	}
+
+	/* Check for a potential next partition definition */
+	if (*mtdparts == ',') {
+		if (partition->size == MTD_SIZE_REMAINING) {
+			printf("No partitions allowed after a fill-up partition\n");
+			return -EINVAL;
+		}
+		++mtdparts;
+	} else if (*mtdparts == ';') {
+		++mtdparts;
+	} else if (*mtdparts == '\0') {
+		/* NOP */
+	} else {
+		printf("Unexpected character '%c' in mtdparts\n", *mtdparts);
+		return -EINVAL;
+	}
+
+	/*
+	 * Allocate a buffer for the name and either copy the provided name or
+	 * auto-generate it with the form 'size at offset'.
+	 */
+	buf = malloc(name_len);
+	if (!buf)
+		return -ENOMEM;
+
+	if (name)
+		strncpy(buf, name, name_len - 1);
+	else
+		snprintf(buf, name_len, "0x%08llx at 0x%08llx",
+			 partition->size, partition->offset);
+
+	buf[name_len - 1] = '\0';
+	partition->name = buf;
+
+	*_mtdparts = mtdparts;
+
+	return 0;
+}
+
+/**
+ * mtdparts_parse_part - Create a partition array from an mtdparts definition
+ *
+ * Stateless function that takes a @parent MTD device, a string @_mtdparts
+ * describing the partitions (with the "mtdparts" command syntax) and creates
+ * the corresponding MTD partition structure array @_parts. Both the name and
+ * the structure partition itself must be freed freed by the caller.
+ *
+ * @parent: MTD device which contains the partitions
+ * @_mtdparts: Pointer to a string describing the partitions with "mtdparts"
+ *             command syntax.
+ * @_parts: Allocated array containing the partitions, must be freed by the
+ *          caller.
+ * @_nb_parts: Size of @_parts array.
+ *
+ * @return 0 on success, an error otherwise.
+ */
+int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
+			 struct mtd_partition **_parts, int *_nb_parts)
+{
+	struct mtd_partition partition = {}, *parts;
+	const char *mtdparts = *_mtdparts;
+	int cur_off = 0, cur_sz = 0;
+	int nb_parts = 0;
+	int ret, idx;
+	u64 sz;
+
+	/* First, iterate over the partitions until we know their number */
+	while (mtdparts[0] != '\0' && mtdparts[0] != ';') {
+		ret = mtd_parse_partition(&mtdparts, &partition);
+		if (ret)
+			return ret;
+
+		free((char *)partition.name);
+		nb_parts++;
+	}
+
+	/* Allocate an array of partitions to give back to the caller */
+	parts = malloc(sizeof(*parts) * nb_parts);
+	if (!parts) {
+		printf("Could not allocate enough space to save partitions meta-data\n");
+		return -ENOMEM;
+	}
+
+	/* Iterate again over each partition to save the data in our array */
+	for (idx = 0; idx < nb_parts; idx++) {
+		ret = mtd_parse_partition(_mtdparts, &parts[idx]);
+		if (ret)
+			return ret;
+
+		if (parts[idx].size == MTD_SIZE_REMAINING)
+			parts[idx].size = parent->size - cur_sz;
+		cur_sz += parts[idx].size;
+
+		sz = parts[idx].size;
+		if (sz < parent->writesize || do_div(sz, parent->writesize)) {
+			printf("Partition size must be a multiple of %d\n",
+			       parent->writesize);
+			return -EINVAL;
+		}
+
+		if (parts[idx].offset == MTD_OFFSET_NOT_SPECIFIED)
+			parts[idx].offset = cur_off;
+		cur_off += parts[idx].size;
+
+		parts[idx].ecclayout = parent->ecclayout;
+	}
+
+	/* Offset by one mtdparts to point to the next device if any */
+	if (*_mtdparts[0] == ';')
+		_mtdparts++;
+
+	*_parts = parts;
+	*_nb_parts = nb_parts;
+
+	return 0;
+}
+
 /*
  * MTD methods which simply translate the effective address and pass through
  * to the _real_ device.
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index ce0e8dbee4..ed4ece5e13 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -86,5 +86,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 		      long long offset, long long length);
 int mtd_del_partition(struct mtd_info *master, int partno);
 uint64_t mtd_get_device_size(const struct mtd_info *mtd);
+int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
+			 struct mtd_partition **_parts, int *_nb_parts);
 
 #endif
-- 
2.17.1

[U-Boot] [PATCH v7 08/13] mtd: uclass: search for an equivalent MTD name with the mtdids

[Miquel Raynal]

Using an MTD device (resp. partition) name in mtdparts is simple and
straightforward. However, for a long time already, another name was
given in mtdparts to indicate a device (resp. partition) so the
"mtdids" environment variable was created to do the match.

Let's create a function that, from an MTD device (resp. partition)
name, search for the equivalent name in the "mtdparts" environment
variable thanks to the "mtdids" string.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/mtdpart.c          | 62 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/partitions.h |  2 ++
 2 files changed, 64 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index d9708da2ae..096351e48b 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -263,6 +263,68 @@ int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
 	return 0;
 }
 
+/**
+ * mtd_search_alternate_name - Search an alternate name for @mtdname thanks to
+ *                             the mtdids legacy environment variable.
+ *
+ * The mtdids string is a list of comma-separated 'dev_id=mtd_id' tupples.
+ * Check if one of the mtd_id matches mtdname, in this case save dev_id in
+ * altname.
+ *
+ * @mtdname: Current MTD device name
+ * @altname: Alternate name to return
+ * @max_len: Length of the alternate name buffer
+ *
+ * @return 0 on success, an error otherwise.
+ */
+int mtd_search_alternate_name(const char *mtdname, char *altname,
+			      unsigned int max_len)
+{
+	const char *mtdids, *equal, *comma, *dev_id, *mtd_id;
+	int dev_id_len, mtd_id_len;
+
+	mtdids = env_get("mtdids");
+	if (!mtdids)
+		return -EINVAL;
+
+	do {
+		/* Find the '=' sign */
+		dev_id = mtdids;
+		equal = strchr(dev_id, '=');
+		if (!equal)
+			break;
+
+		dev_id_len = equal - mtdids;
+		mtd_id = equal + 1;
+
+		/* Find the end of the tupple */
+		comma = strchr(mtdids, ',');
+		if (comma)
+			mtd_id_len = comma - equal;
+		else
+			mtd_id_len = &mtdids[strlen(mtdids)] - equal;
+
+		if (!dev_id_len || !mtd_id_len)
+			return -EINVAL;
+
+		if (dev_id_len + 1 > max_len)
+			continue;
+
+		/* Compare the name we search with the current mtd_id */
+		if (!strncmp(mtdname, mtd_id, mtd_id_len)) {
+			strncpy(altname, dev_id, dev_id_len);
+			altname[dev_id_len] = 0;
+
+			return 0;
+		}
+
+		/* Go to the next tupple */
+		mtdids = comma + 1;
+	} while (comma);
+
+	return -EINVAL;
+}
+
 /*
  * MTD methods which simply translate the effective address and pass through
  * to the _real_ device.
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index ed4ece5e13..082a4966ea 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -88,5 +88,7 @@ int mtd_del_partition(struct mtd_info *master, int partno);
 uint64_t mtd_get_device_size(const struct mtd_info *mtd);
 int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
 			 struct mtd_partition **_parts, int *_nb_parts);
+int mtd_search_alternate_name(const char *mtdname, char *altname,
+			      unsigned int max_len);
 
 #endif
-- 
2.17.1

[U-Boot] [PATCH v7 09/13] mtd: mtdpart: implement proper partition handling

[Miquel Raynal]

Instead of collecting partitions in a flat list, create a hierarchy
within the mtd_info structure: use a partitions list to keep track of
the partitions of an MTD device (which might be itself a partition of
another MTD device), a pointer to the parent device (NULL when the MTD
device is the root one, not a partition).

By also saving directly in mtd_info the offset of the partition, we
can get rid of the mtd_part structure.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/mtdcore.c          |   2 +
 drivers/mtd/mtdpart.c          | 412 ++++++++++++++-------------------
 include/linux/mtd/mtd.h        |  31 +++
 include/linux/mtd/partitions.h |   1 -
 4 files changed, 208 insertions(+), 238 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index fb1d68d5e2..fb6c779abb 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -426,6 +426,8 @@ int add_mtd_device(struct mtd_info *mtd)
 	mtd->index = i;
 	mtd->usecount = 0;
 
+	INIT_LIST_HEAD(&mtd->partitions);
+
 	/* default value if not set by driver */
 	if (mtd->bitflip_threshold == 0)
 		mtd->bitflip_threshold = mtd->ecc_strength;
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 096351e48b..afc02dcb24 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -29,29 +29,12 @@
 
 #include "mtdcore.h"
 
-/* Our partition linked list */
-static LIST_HEAD(mtd_partitions);
 #ifndef __UBOOT__
 static DEFINE_MUTEX(mtd_partitions_mutex);
 #else
 DEFINE_MUTEX(mtd_partitions_mutex);
 #endif
 
-/* Our partition node structure */
-struct mtd_part {
-	struct mtd_info mtd;
-	struct mtd_info *master;
-	uint64_t offset;
-	struct list_head list;
-};
-
-/*
- * Given a pointer to the MTD object in the mtd_part structure, we can retrieve
- * the pointer to that structure with this macro.
- */
-#define PART(x)  ((struct mtd_part *)(x))
-
-
 #ifdef __UBOOT__
 /* from mm/util.c */
 
@@ -333,19 +316,18 @@ int mtd_search_alternate_name(const char *mtdname, char *altname,
 static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
 		size_t *retlen, u_char *buf)
 {
-	struct mtd_part *part = PART(mtd);
 	struct mtd_ecc_stats stats;
 	int res;
 
-	stats = part->master->ecc_stats;
-	res = part->master->_read(part->master, from + part->offset, len,
-				  retlen, buf);
+	stats = mtd->parent->ecc_stats;
+	res = mtd->parent->_read(mtd->parent, from + mtd->offset, len,
+				 retlen, buf);
 	if (unlikely(mtd_is_eccerr(res)))
 		mtd->ecc_stats.failed +=
-			part->master->ecc_stats.failed - stats.failed;
+			mtd->parent->ecc_stats.failed - stats.failed;
 	else
 		mtd->ecc_stats.corrected +=
-			part->master->ecc_stats.corrected - stats.corrected;
+			mtd->parent->ecc_stats.corrected - stats.corrected;
 	return res;
 }
 
@@ -353,17 +335,13 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
 static int part_point(struct mtd_info *mtd, loff_t from, size_t len,
 		size_t *retlen, void **virt, resource_size_t *phys)
 {
-	struct mtd_part *part = PART(mtd);
-
-	return part->master->_point(part->master, from + part->offset, len,
-				    retlen, virt, phys);
+	return mtd->parent->_point(mtd->parent, from + mtd->offset, len,
+				   retlen, virt, phys);
 }
 
 static int part_unpoint(struct mtd_info *mtd, loff_t from, size_t len)
 {
-	struct mtd_part *part = PART(mtd);
-
-	return part->master->_unpoint(part->master, from + part->offset, len);
+	return mtd->parent->_unpoint(mtd->parent, from + mtd->offset, len);
 }
 #endif
 
@@ -372,17 +350,13 @@ static unsigned long part_get_unmapped_area(struct mtd_info *mtd,
 					    unsigned long offset,
 					    unsigned long flags)
 {
-	struct mtd_part *part = PART(mtd);
-
-	offset += part->offset;
-	return part->master->_get_unmapped_area(part->master, len, offset,
-						flags);
+	offset += mtd->offset;
+	return mtd->parent->_get_unmapped_area(mtd->parent, len, offset, flags);
 }
 
 static int part_read_oob(struct mtd_info *mtd, loff_t from,
 		struct mtd_oob_ops *ops)
 {
-	struct mtd_part *part = PART(mtd);
 	int res;
 
 	if (from >= mtd->size)
@@ -407,7 +381,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 			return -EINVAL;
 	}
 
-	res = part->master->_read_oob(part->master, from + part->offset, ops);
+	res = mtd->parent->_read_oob(mtd->parent, from + mtd->offset, ops);
 	if (unlikely(res)) {
 		if (mtd_is_bitflip(res))
 			mtd->ecc_stats.corrected++;
@@ -420,99 +394,87 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 static int part_read_user_prot_reg(struct mtd_info *mtd, loff_t from,
 		size_t len, size_t *retlen, u_char *buf)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_read_user_prot_reg(part->master, from, len,
-						 retlen, buf);
+	return mtd->parent->_read_user_prot_reg(mtd->parent, from, len,
+						retlen, buf);
 }
 
 static int part_get_user_prot_info(struct mtd_info *mtd, size_t len,
 				   size_t *retlen, struct otp_info *buf)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_get_user_prot_info(part->master, len, retlen,
-						 buf);
+	return mtd->parent->_get_user_prot_info(mtd->parent, len, retlen,
+						buf);
 }
 
 static int part_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
 		size_t len, size_t *retlen, u_char *buf)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_read_fact_prot_reg(part->master, from, len,
-						 retlen, buf);
+	return mtd->parent->_read_fact_prot_reg(mtd->parent, from, len,
+						retlen, buf);
 }
 
 static int part_get_fact_prot_info(struct mtd_info *mtd, size_t len,
 				   size_t *retlen, struct otp_info *buf)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_get_fact_prot_info(part->master, len, retlen,
-						 buf);
+	return mtd->parent->_get_fact_prot_info(mtd->parent, len, retlen,
+						buf);
 }
 
 static int part_write(struct mtd_info *mtd, loff_t to, size_t len,
 		size_t *retlen, const u_char *buf)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_write(part->master, to + part->offset, len,
-				    retlen, buf);
+	return mtd->parent->_write(mtd->parent, to + mtd->offset, len,
+				   retlen, buf);
 }
 
 static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
 		size_t *retlen, const u_char *buf)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_panic_write(part->master, to + part->offset, len,
-					  retlen, buf);
+	return mtd->parent->_panic_write(mtd->parent, to + mtd->offset, len,
+					 retlen, buf);
 }
 
 static int part_write_oob(struct mtd_info *mtd, loff_t to,
 		struct mtd_oob_ops *ops)
 {
-	struct mtd_part *part = PART(mtd);
-
 	if (to >= mtd->size)
 		return -EINVAL;
 	if (ops->datbuf && to + ops->len > mtd->size)
 		return -EINVAL;
-	return part->master->_write_oob(part->master, to + part->offset, ops);
+	return mtd->parent->_write_oob(mtd->parent, to + mtd->offset, ops);
 }
 
 static int part_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
 		size_t len, size_t *retlen, u_char *buf)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_write_user_prot_reg(part->master, from, len,
-						  retlen, buf);
+	return mtd->parent->_write_user_prot_reg(mtd->parent, from, len,
+						 retlen, buf);
 }
 
 static int part_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
 		size_t len)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_lock_user_prot_reg(part->master, from, len);
+	return mtd->parent->_lock_user_prot_reg(mtd->parent, from, len);
 }
 
 #ifndef __UBOOT__
 static int part_writev(struct mtd_info *mtd, const struct kvec *vecs,
 		unsigned long count, loff_t to, size_t *retlen)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_writev(part->master, vecs, count,
-				     to + part->offset, retlen);
+	return mtd->parent->_writev(mtd->parent, vecs, count,
+				    to + mtd->offset, retlen);
 }
 #endif
 
 static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
-	struct mtd_part *part = PART(mtd);
 	int ret;
 
-	instr->addr += part->offset;
-	ret = part->master->_erase(part->master, instr);
+	instr->addr += mtd->offset;
+	ret = mtd->parent->_erase(mtd->parent, instr);
 	if (ret) {
 		if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
-			instr->fail_addr -= part->offset;
-		instr->addr -= part->offset;
+			instr->fail_addr -= mtd->offset;
+		instr->addr -= mtd->offset;
 	}
 	return ret;
 }
@@ -520,11 +482,9 @@ static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
 void mtd_erase_callback(struct erase_info *instr)
 {
 	if (instr->mtd->_erase == part_erase) {
-		struct mtd_part *part = PART(instr->mtd);
-
 		if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
-			instr->fail_addr -= part->offset;
-		instr->addr -= part->offset;
+			instr->fail_addr -= instr->mtd->offset;
+		instr->addr -= instr->mtd->offset;
 	}
 	if (instr->callback)
 		instr->callback(instr);
@@ -533,107 +493,112 @@ EXPORT_SYMBOL_GPL(mtd_erase_callback);
 
 static int part_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_lock(part->master, ofs + part->offset, len);
+	return mtd->parent->_lock(mtd->parent, ofs + mtd->offset, len);
 }
 
 static int part_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_unlock(part->master, ofs + part->offset, len);
+	return mtd->parent->_unlock(mtd->parent, ofs + mtd->offset, len);
 }
 
 static int part_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_is_locked(part->master, ofs + part->offset, len);
+	return mtd->parent->_is_locked(mtd->parent, ofs + mtd->offset, len);
 }
 
 static void part_sync(struct mtd_info *mtd)
 {
-	struct mtd_part *part = PART(mtd);
-	part->master->_sync(part->master);
+	mtd->parent->_sync(mtd->parent);
 }
 
 #ifndef __UBOOT__
 static int part_suspend(struct mtd_info *mtd)
 {
-	struct mtd_part *part = PART(mtd);
-	return part->master->_suspend(part->master);
+	return mtd->parent->_suspend(mtd->parent);
 }
 
 static void part_resume(struct mtd_info *mtd)
 {
-	struct mtd_part *part = PART(mtd);
-	part->master->_resume(part->master);
+	mtd->parent->_resume(mtd->parent);
 }
 #endif
 
 static int part_block_isreserved(struct mtd_info *mtd, loff_t ofs)
 {
-	struct mtd_part *part = PART(mtd);
-	ofs += part->offset;
-	return part->master->_block_isreserved(part->master, ofs);
+	ofs += mtd->offset;
+	return mtd->parent->_block_isreserved(mtd->parent, ofs);
 }
 
 static int part_block_isbad(struct mtd_info *mtd, loff_t ofs)
 {
-	struct mtd_part *part = PART(mtd);
-	ofs += part->offset;
-	return part->master->_block_isbad(part->master, ofs);
+	ofs += mtd->offset;
+	return mtd->parent->_block_isbad(mtd->parent, ofs);
 }
 
 static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
 {
-	struct mtd_part *part = PART(mtd);
 	int res;
 
-	ofs += part->offset;
-	res = part->master->_block_markbad(part->master, ofs);
+	ofs += mtd->offset;
+	res = mtd->parent->_block_markbad(mtd->parent, ofs);
 	if (!res)
 		mtd->ecc_stats.badblocks++;
 	return res;
 }
 
-static inline void free_partition(struct mtd_part *p)
+static inline void free_partition(struct mtd_info *p)
 {
-	kfree(p->mtd.name);
+	kfree(p->name);
 	kfree(p);
 }
 
 /*
  * This function unregisters and destroy all slave MTD objects which are
- * attached to the given master MTD object.
+ * attached to the given master MTD object, recursively.
  */
-
-int del_mtd_partitions(struct mtd_info *master)
+static int do_del_mtd_partitions(struct mtd_info *master)
 {
-	struct mtd_part *slave, *next;
+	struct mtd_info *slave, *next;
 	int ret, err = 0;
 
-	debug("Deleting MTD partitions on \"%s\":\n", master->name);
+	list_for_each_entry_safe(slave, next, &master->partitions, node) {
+		if (mtd_has_partitions(slave))
+			del_mtd_partitions(slave);
 
-	mutex_lock(&mtd_partitions_mutex);
-	list_for_each_entry_safe(slave, next, &mtd_partitions, list)
-		if (slave->master == master) {
-			ret = del_mtd_device(&slave->mtd);
-			if (ret < 0) {
-				err = ret;
-				continue;
-			}
-			list_del(&slave->list);
-			free_partition(slave);
+		debug("Deleting %s MTD partition\n", slave->name);
+		ret = del_mtd_device(slave);
+		if (ret < 0) {
+			printk("Error when deleting partition \"%s\" (%d)\n",
+			       slave->name, ret);
+			err = ret;
+			continue;
 		}
-	mutex_unlock(&mtd_partitions_mutex);
+
+		list_del(&slave->node);
+		free_partition(slave);
+	}
 
 	return err;
 }
 
-static struct mtd_part *allocate_partition(struct mtd_info *master,
-			const struct mtd_partition *part, int partno,
-			uint64_t cur_offset)
+int del_mtd_partitions(struct mtd_info *master)
 {
-	struct mtd_part *slave;
+	int ret;
+
+	debug("Deleting MTD partitions on \"%s\":\n", master->name);
+
+	mutex_lock(&mtd_partitions_mutex);
+	ret = do_del_mtd_partitions(master);
+	mutex_unlock(&mtd_partitions_mutex);
+
+	return ret;
+}
+
+static struct mtd_info *allocate_partition(struct mtd_info *master,
+					   const struct mtd_partition *part,
+					   int partno, uint64_t cur_offset)
+{
+	struct mtd_info *slave;
 	char *name;
 
 	/* allocate the partition structure */
@@ -648,85 +613,87 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 	}
 
 	/* set up the MTD object for this partition */
-	slave->mtd.type = master->type;
-	slave->mtd.flags = master->flags & ~part->mask_flags;
-	slave->mtd.size = part->size;
-	slave->mtd.writesize = master->writesize;
-	slave->mtd.writebufsize = master->writebufsize;
-	slave->mtd.oobsize = master->oobsize;
-	slave->mtd.oobavail = master->oobavail;
-	slave->mtd.subpage_sft = master->subpage_sft;
+	slave->type = master->type;
+	slave->flags = master->flags & ~part->mask_flags;
+	slave->size = part->size;
+	slave->writesize = master->writesize;
+	slave->writebufsize = master->writebufsize;
+	slave->oobsize = master->oobsize;
+	slave->oobavail = master->oobavail;
+	slave->subpage_sft = master->subpage_sft;
 
-	slave->mtd.name = name;
-	slave->mtd.owner = master->owner;
+	slave->name = name;
+	slave->owner = master->owner;
 #ifndef __UBOOT__
-	slave->mtd.backing_dev_info = master->backing_dev_info;
+	slave->backing_dev_info = master->backing_dev_info;
 
 	/* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
 	 * to have the same data be in two different partitions.
 	 */
-	slave->mtd.dev.parent = master->dev.parent;
+	slave->dev.parent = master->dev.parent;
 #endif
 
 	if (master->_read)
-		slave->mtd._read = part_read;
+		slave->_read = part_read;
 	if (master->_write)
-		slave->mtd._write = part_write;
+		slave->_write = part_write;
 
 	if (master->_panic_write)
-		slave->mtd._panic_write = part_panic_write;
+		slave->_panic_write = part_panic_write;
 
 #ifndef __UBOOT__
 	if (master->_point && master->_unpoint) {
-		slave->mtd._point = part_point;
-		slave->mtd._unpoint = part_unpoint;
+		slave->_point = part_point;
+		slave->_unpoint = part_unpoint;
 	}
 #endif
 
 	if (master->_get_unmapped_area)
-		slave->mtd._get_unmapped_area = part_get_unmapped_area;
+		slave->_get_unmapped_area = part_get_unmapped_area;
 	if (master->_read_oob)
-		slave->mtd._read_oob = part_read_oob;
+		slave->_read_oob = part_read_oob;
 	if (master->_write_oob)
-		slave->mtd._write_oob = part_write_oob;
+		slave->_write_oob = part_write_oob;
 	if (master->_read_user_prot_reg)
-		slave->mtd._read_user_prot_reg = part_read_user_prot_reg;
+		slave->_read_user_prot_reg = part_read_user_prot_reg;
 	if (master->_read_fact_prot_reg)
-		slave->mtd._read_fact_prot_reg = part_read_fact_prot_reg;
+		slave->_read_fact_prot_reg = part_read_fact_prot_reg;
 	if (master->_write_user_prot_reg)
-		slave->mtd._write_user_prot_reg = part_write_user_prot_reg;
+		slave->_write_user_prot_reg = part_write_user_prot_reg;
 	if (master->_lock_user_prot_reg)
-		slave->mtd._lock_user_prot_reg = part_lock_user_prot_reg;
+		slave->_lock_user_prot_reg = part_lock_user_prot_reg;
 	if (master->_get_user_prot_info)
-		slave->mtd._get_user_prot_info = part_get_user_prot_info;
+		slave->_get_user_prot_info = part_get_user_prot_info;
 	if (master->_get_fact_prot_info)
-		slave->mtd._get_fact_prot_info = part_get_fact_prot_info;
+		slave->_get_fact_prot_info = part_get_fact_prot_info;
 	if (master->_sync)
-		slave->mtd._sync = part_sync;
+		slave->_sync = part_sync;
 #ifndef __UBOOT__
 	if (!partno && !master->dev.class && master->_suspend &&
 	    master->_resume) {
-			slave->mtd._suspend = part_suspend;
-			slave->mtd._resume = part_resume;
+			slave->_suspend = part_suspend;
+			slave->_resume = part_resume;
 	}
 	if (master->_writev)
-		slave->mtd._writev = part_writev;
+		slave->_writev = part_writev;
 #endif
 	if (master->_lock)
-		slave->mtd._lock = part_lock;
+		slave->_lock = part_lock;
 	if (master->_unlock)
-		slave->mtd._unlock = part_unlock;
+		slave->_unlock = part_unlock;
 	if (master->_is_locked)
-		slave->mtd._is_locked = part_is_locked;
+		slave->_is_locked = part_is_locked;
 	if (master->_block_isreserved)
-		slave->mtd._block_isreserved = part_block_isreserved;
+		slave->_block_isreserved = part_block_isreserved;
 	if (master->_block_isbad)
-		slave->mtd._block_isbad = part_block_isbad;
+		slave->_block_isbad = part_block_isbad;
 	if (master->_block_markbad)
-		slave->mtd._block_markbad = part_block_markbad;
-	slave->mtd._erase = part_erase;
-	slave->master = master;
+		slave->_block_markbad = part_block_markbad;
+	slave->_erase = part_erase;
+	slave->parent = master;
 	slave->offset = part->offset;
+	INIT_LIST_HEAD(&slave->partitions);
+	INIT_LIST_HEAD(&slave->node);
 
 	if (slave->offset == MTDPART_OFS_APPEND)
 		slave->offset = cur_offset;
@@ -742,41 +709,41 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 	}
 	if (slave->offset == MTDPART_OFS_RETAIN) {
 		slave->offset = cur_offset;
-		if (master->size - slave->offset >= slave->mtd.size) {
-			slave->mtd.size = master->size - slave->offset
-							- slave->mtd.size;
+		if (master->size - slave->offset >= slave->size) {
+			slave->size = master->size - slave->offset
+							- slave->size;
 		} else {
 			debug("mtd partition \"%s\" doesn't have enough space: %#llx < %#llx, disabled\n",
 				part->name, master->size - slave->offset,
-				slave->mtd.size);
+				slave->size);
 			/* register to preserve ordering */
 			goto out_register;
 		}
 	}
-	if (slave->mtd.size == MTDPART_SIZ_FULL)
-		slave->mtd.size = master->size - slave->offset;
+	if (slave->size == MTDPART_SIZ_FULL)
+		slave->size = master->size - slave->offset;
 
 	debug("0x%012llx-0x%012llx : \"%s\"\n", (unsigned long long)slave->offset,
-		(unsigned long long)(slave->offset + slave->mtd.size), slave->mtd.name);
+		(unsigned long long)(slave->offset + slave->size), slave->name);
 
 	/* let's do some sanity checks */
 	if (slave->offset >= master->size) {
 		/* let's register it anyway to preserve ordering */
 		slave->offset = 0;
-		slave->mtd.size = 0;
+		slave->size = 0;
 		printk(KERN_ERR"mtd: partition \"%s\" is out of reach -- disabled\n",
 			part->name);
 		goto out_register;
 	}
-	if (slave->offset + slave->mtd.size > master->size) {
-		slave->mtd.size = master->size - slave->offset;
+	if (slave->offset + slave->size > master->size) {
+		slave->size = master->size - slave->offset;
 		printk(KERN_WARNING"mtd: partition \"%s\" extends beyond the end of device \"%s\" -- size truncated to %#llx\n",
-			part->name, master->name, (unsigned long long)slave->mtd.size);
+			part->name, master->name, (unsigned long long)slave->size);
 	}
 	if (master->numeraseregions > 1) {
 		/* Deal with variable erase size stuff */
 		int i, max = master->numeraseregions;
-		u64 end = slave->offset + slave->mtd.size;
+		u64 end = slave->offset + slave->size;
 		struct mtd_erase_region_info *regions = master->eraseregions;
 
 		/* Find the first erase regions which is part of this
@@ -789,44 +756,44 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 
 		/* Pick biggest erasesize */
 		for (; i < max && regions[i].offset < end; i++) {
-			if (slave->mtd.erasesize < regions[i].erasesize) {
-				slave->mtd.erasesize = regions[i].erasesize;
+			if (slave->erasesize < regions[i].erasesize) {
+				slave->erasesize = regions[i].erasesize;
 			}
 		}
-		BUG_ON(slave->mtd.erasesize == 0);
+		BUG_ON(slave->erasesize == 0);
 	} else {
 		/* Single erase size */
-		slave->mtd.erasesize = master->erasesize;
+		slave->erasesize = master->erasesize;
 	}
 
-	if ((slave->mtd.flags & MTD_WRITEABLE) &&
-	    mtd_mod_by_eb(slave->offset, &slave->mtd)) {
+	if ((slave->flags & MTD_WRITEABLE) &&
+	    mtd_mod_by_eb(slave->offset, slave)) {
 		/* Doesn't start on a boundary of major erase size */
 		/* FIXME: Let it be writable if it is on a boundary of
 		 * _minor_ erase size though */
-		slave->mtd.flags &= ~MTD_WRITEABLE;
+		slave->flags &= ~MTD_WRITEABLE;
 		printk(KERN_WARNING"mtd: partition \"%s\" doesn't start on an erase block boundary -- force read-only\n",
 			part->name);
 	}
-	if ((slave->mtd.flags & MTD_WRITEABLE) &&
-	    mtd_mod_by_eb(slave->mtd.size, &slave->mtd)) {
-		slave->mtd.flags &= ~MTD_WRITEABLE;
+	if ((slave->flags & MTD_WRITEABLE) &&
+	    mtd_mod_by_eb(slave->size, slave)) {
+		slave->flags &= ~MTD_WRITEABLE;
 		printk(KERN_WARNING"mtd: partition \"%s\" doesn't end on an erase block -- force read-only\n",
 			part->name);
 	}
 
-	slave->mtd.ecclayout = master->ecclayout;
-	slave->mtd.ecc_step_size = master->ecc_step_size;
-	slave->mtd.ecc_strength = master->ecc_strength;
-	slave->mtd.bitflip_threshold = master->bitflip_threshold;
+	slave->ecclayout = master->ecclayout;
+	slave->ecc_step_size = master->ecc_step_size;
+	slave->ecc_strength = master->ecc_strength;
+	slave->bitflip_threshold = master->bitflip_threshold;
 
 	if (master->_block_isbad) {
 		uint64_t offs = 0;
 
-		while (offs < slave->mtd.size) {
+		while (offs < slave->size) {
 			if (mtd_block_isbad(master, offs + slave->offset))
-				slave->mtd.ecc_stats.badblocks++;
-			offs += slave->mtd.erasesize;
+				slave->ecc_stats.badblocks++;
+			offs += slave->erasesize;
 		}
 	}
 
@@ -839,7 +806,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 		      long long offset, long long length)
 {
 	struct mtd_partition part;
-	struct mtd_part *p, *new;
+	struct mtd_info *p, *new;
 	uint64_t start, end;
 	int ret = 0;
 
@@ -868,21 +835,20 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 	end = offset + length;
 
 	mutex_lock(&mtd_partitions_mutex);
-	list_for_each_entry(p, &mtd_partitions, list)
-		if (p->master == master) {
-			if ((start >= p->offset) &&
-			    (start < (p->offset + p->mtd.size)))
-				goto err_inv;
+	list_for_each_entry(p, &master->partitions, node) {
+		if ((start >= p->offset) &&
+		    (start < (p->offset + p->size)))
+			goto err_inv;
 
-			if ((end >= p->offset) &&
-			    (end < (p->offset + p->mtd.size)))
-				goto err_inv;
-		}
+		if ((end >= p->offset) &&
+		    (end < (p->offset + p->size)))
+			goto err_inv;
+	}
 
-	list_add(&new->list, &mtd_partitions);
+	list_add_tail(&new->node, &master->partitions);
 	mutex_unlock(&mtd_partitions_mutex);
 
-	add_mtd_device(&new->mtd);
+	add_mtd_device(new);
 
 	return ret;
 err_inv:
@@ -894,18 +860,17 @@ EXPORT_SYMBOL_GPL(mtd_add_partition);
 
 int mtd_del_partition(struct mtd_info *master, int partno)
 {
-	struct mtd_part *slave, *next;
+	struct mtd_info *slave, *next;
 	int ret = -EINVAL;
 
 	mutex_lock(&mtd_partitions_mutex);
-	list_for_each_entry_safe(slave, next, &mtd_partitions, list)
-		if ((slave->master == master) &&
-		    (slave->mtd.index == partno)) {
-			ret = del_mtd_device(&slave->mtd);
+	list_for_each_entry_safe(slave, next, &master->partitions, node)
+		if (slave->index == partno) {
+			ret = del_mtd_device(slave);
 			if (ret < 0)
 				break;
 
-			list_del(&slave->list);
+			list_del(&slave->node);
 			free_partition(slave);
 			break;
 		}
@@ -929,20 +894,10 @@ int add_mtd_partitions(struct mtd_info *master,
 		       const struct mtd_partition *parts,
 		       int nbparts)
 {
-	struct mtd_part *slave;
+	struct mtd_info *slave;
 	uint64_t cur_offset = 0;
 	int i;
 
-#ifdef __UBOOT__
-	/*
-	 * Need to init the list here, since LIST_INIT() does not
-	 * work on platforms where relocation has problems (like MIPS
-	 * & PPC).
-	 */
-	if (mtd_partitions.next == NULL)
-		INIT_LIST_HEAD(&mtd_partitions);
-#endif
-
 	debug("Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
 
 	for (i = 0; i < nbparts; i++) {
@@ -951,12 +906,12 @@ int add_mtd_partitions(struct mtd_info *master,
 			return PTR_ERR(slave);
 
 		mutex_lock(&mtd_partitions_mutex);
-		list_add(&slave->list, &mtd_partitions);
+		list_add_tail(&slave->node, &master->partitions);
 		mutex_unlock(&mtd_partitions_mutex);
 
-		add_mtd_device(&slave->mtd);
+		add_mtd_device(slave);
 
-		cur_offset = slave->offset + slave->mtd.size;
+		cur_offset = slave->offset + slave->size;
 	}
 
 	return 0;
@@ -1059,29 +1014,12 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 }
 #endif
 
-int mtd_is_partition(const struct mtd_info *mtd)
-{
-	struct mtd_part *part;
-	int ispart = 0;
-
-	mutex_lock(&mtd_partitions_mutex);
-	list_for_each_entry(part, &mtd_partitions, list)
-		if (&part->mtd == mtd) {
-			ispart = 1;
-			break;
-		}
-	mutex_unlock(&mtd_partitions_mutex);
-
-	return ispart;
-}
-EXPORT_SYMBOL_GPL(mtd_is_partition);
-
 /* Returns the size of the entire flash chip */
 uint64_t mtd_get_device_size(const struct mtd_info *mtd)
 {
-	if (!mtd_is_partition(mtd))
-		return mtd->size;
+	if (mtd_is_partition(mtd))
+		return mtd->parent->size;
 
-	return PART(mtd)->master->size;
+	return mtd->size;
 }
 EXPORT_SYMBOL_GPL(mtd_get_device_size);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index b8c2c3fd59..ba582d38bc 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -307,6 +307,27 @@ struct mtd_info {
 	struct udevice *dev;
 #endif
 	int usecount;
+
+	/* MTD devices do not have any parent. MTD partitions do. */
+	struct mtd_info *parent;
+
+	/*
+	 * Offset of the partition relatively to the parent offset.
+	 * Is 0 for real MTD devices (ie. not partitions).
+	 */
+	u64 offset;
+
+	/*
+	 * List node used to add an MTD partition to the parent
+	 * partition list.
+	 */
+	struct list_head node;
+
+	/*
+	 * List of partitions attached to this MTD device (the parent
+	 * MTD device can itself be a partition).
+	 */
+	struct list_head partitions;
 };
 
 #if IS_ENABLED(CONFIG_DM)
@@ -334,6 +355,16 @@ static inline const struct device_node *mtd_get_of_node(struct mtd_info *mtd)
 }
 #endif
 
+static inline bool mtd_is_partition(const struct mtd_info *mtd)
+{
+	return mtd->parent;
+}
+
+static inline bool mtd_has_partitions(const struct mtd_info *mtd)
+{
+	return !list_empty(&mtd->partitions);
+}
+
 int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,
 		      struct mtd_oob_region *oobecc);
 int mtd_ooblayout_find_eccregion(struct mtd_info *mtd, int eccbyte,
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index 082a4966ea..b8db1b0a58 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -81,7 +81,6 @@ extern void register_mtd_parser(struct mtd_part_parser *parser);
 extern void deregister_mtd_parser(struct mtd_part_parser *parser);
 #endif
 
-int mtd_is_partition(const struct mtd_info *mtd);
 int mtd_add_partition(struct mtd_info *master, const char *name,
 		      long long offset, long long length);
 int mtd_del_partition(struct mtd_info *master, int partno);
-- 
2.17.1

[U-Boot] [PATCH v7 10/13] cmd: mtd: add 'mtd' command

[Miquel Raynal]

There should not be a 'nand' command, a 'sf' command and certainly not
a new 'spi-nand' command. Write a 'mtd' command instead to manage all
MTD devices/partitions at once. This should be the preferred way to
access any MTD device.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jagan Teki <jagan@openedev.com>
---
 cmd/Kconfig          |  10 +-
 cmd/Makefile         |   1 +
 cmd/mtd.c            | 517 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/Makefile |   2 +-
 include/mtd.h        |   1 +
 5 files changed, 528 insertions(+), 3 deletions(-)
 create mode 100644 cmd/mtd.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index ef43ed8dda..0778d2ecff 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -847,6 +847,12 @@ config CMD_MMC_SWRITE
 	  Enable support for the "mmc swrite" command to write Android sparse
 	  images to eMMC.
 
+config CMD_MTD
+	bool "mtd"
+	select MTD_PARTITIONS
+	help
+	  MTD commands support.
+
 config CMD_NAND
 	bool "nand"
 	default y if NAND_SUNXI
@@ -1671,14 +1677,14 @@ config CMD_MTDPARTS
 
 config MTDIDS_DEFAULT
 	string "Default MTD IDs"
-	depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
+	depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
 	help
 	  Defines a default MTD IDs list for use with MTD partitions in the
 	  Linux MTD command line partitions format.
 
 config MTDPARTS_DEFAULT
 	string "Default MTD partition scheme"
-	depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
+	depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
 	help
 	  Defines a default MTD partitioning scheme in the Linux MTD command
 	  line partitions format
diff --git a/cmd/Makefile b/cmd/Makefile
index 323f1fd2c7..32fd102189 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -90,6 +90,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..32295fe86c
--- /dev/null
+++ b/cmd/mtd.c
@@ -0,0 +1,517 @@
+// 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 <command.h>
+#include <common.h>
+#include <console.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <malloc.h>
+#include <mapmem.h>
+#include <mtd.h>
+#include <dm/device.h>
+#include <dm/uclass-internal.h>
+
+#define MTD_NAME_MAX_LEN 20
+
+static char *old_mtdparts;
+
+static void mtd_dump_buf(u8 *buf, uint len, uint offset)
+{
+	int i, j;
+
+	for (i = 0; i < len; ) {
+		printf("0x%08x:\t", offset + 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_parts(struct mtd_info *mtd, int level)
+{
+	struct mtd_info *part;
+	int i;
+
+	if (list_empty(&mtd->partitions))
+		return;
+
+	list_for_each_entry(part, &mtd->partitions, node) {
+		for (i = 0; i < level; i++)
+			printf("\t");
+		printf("* %s\n", part->name);
+		for (i = 0; i < level; i++)
+			printf("\t");
+		printf("  > Offset: 0x%llx bytes\n", part->offset);
+		for (i = 0; i < level; i++)
+			printf("\t");
+		printf("  > Size: 0x%llx bytes\n", part->size);
+
+		mtd_show_parts(part, level + 1);
+	}
+}
+
+static void mtd_show_device(struct mtd_info *mtd)
+{
+	/* Device */
+	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");
+
+	/* MTD device information */
+	printf("  > type: ");
+	switch (mtd->type) {
+	case MTD_RAM:
+		printf("RAM\n");
+		break;
+	case MTD_ROM:
+		printf("ROM\n");
+		break;
+	case MTD_NORFLASH:
+		printf("NOR flash\n");
+		break;
+	case MTD_NANDFLASH:
+		printf("NAND flash\n");
+		break;
+	case MTD_DATAFLASH:
+		printf("Data flash\n");
+		break;
+	case MTD_UBIVOLUME:
+		printf("UBI volume\n");
+		break;
+	case MTD_MLCNANDFLASH:
+		printf("MLC NAND flash\n");
+		break;
+	case MTD_ABSENT:
+	default:
+		printf("Unknown\n");
+		break;
+	}
+
+	printf("  > Size: 0x%llx bytes\n", mtd->size);
+	printf("  > Block: 0x%x bytes\n", mtd->erasesize);
+	printf("  > Min I/O: 0x%x bytes\n", mtd->writesize);
+
+	if (mtd->oobsize) {
+		printf("  > OOB size: %u bytes\n", mtd->oobsize);
+		printf("  > OOB available: %u bytes\n", mtd->oobavail);
+	}
+
+	if (mtd->ecc_strength) {
+		printf("  > ECC strength: %u bits\n", mtd->ecc_strength);
+		printf("  > ECC step size: %u bytes\n", mtd->ecc_step_size);
+		printf("  > Bitflip threshold: %u bits\n",
+		       mtd->bitflip_threshold);
+	}
+
+	/* MTD partitions, if any */
+	mtd_show_parts(mtd, 1);
+}
+
+#if IS_ENABLED(CONFIG_MTD)
+static void mtd_probe_uclass_mtd_devs(void)
+{
+	struct udevice *dev;
+	int idx = 0;
+
+	/* Probe devices with DM compliant drivers */
+	while (!uclass_find_device(UCLASS_MTD, idx, &dev) && dev) {
+		mtd_probe(dev);
+		idx++;
+	}
+}
+#else
+static void mtd_probe_uclass_mtd_devs(void) { }
+#endif
+
+int mtd_probe_devices(void)
+{
+	const char *mtdparts = env_get("mtdparts");
+	bool remaining_partitions = true;
+	struct mtd_info *mtd;
+	int i;
+
+	mtd_probe_uclass_mtd_devs();
+
+	/* Check if mtdparts changed since last call, if not, just exit */
+	if (!strcmp(mtdparts, old_mtdparts))
+		return CMD_RET_SUCCESS;
+
+	/* Update the local copy of mtdparts */
+	free(old_mtdparts);
+	old_mtdparts = strdup(mtdparts);
+
+	/* If at least one partition is still in use, do not delete anything */
+	mtd_for_each_device(mtd) {
+		if (mtd->usecount) {
+			printf("Partition \"%s\" already in use, aborting\n",
+			       mtd->name);
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	/*
+	 * Everything looks clear, remove all partitions. It is not safe to
+	 * remove entries from the mtd_for_each_device loop as it uses idr
+	 * indexes and the partitions removal is done in bulk (all partitions of
+	 * one device at the same time), so break and iterate from start each
+	 * time a new partition is found and deleted.
+	 */
+	while (remaining_partitions) {
+		remaining_partitions = false;
+		mtd_for_each_device(mtd) {
+			if (!mtd_is_partition(mtd) && mtd_has_partitions(mtd)) {
+				del_mtd_partitions(mtd);
+				remaining_partitions = true;
+				break;
+			}
+		}
+	}
+
+	/* Start the parsing by ignoring the extra 'mtdparts=' prefix, if any */
+	if (strstr(mtdparts, "mtdparts="))
+		mtdparts += 9;
+
+	/* For each MTD device in mtdparts */
+	while (mtdparts[0] != '\0') {
+		char mtdname[MTD_NAME_MAX_LEN], *colon;
+		struct mtd_partition *parts;
+		int mtdname_len, len;
+		int ret;
+
+		colon = strchr(mtdparts, ':');
+		if (!colon) {
+			printf("Wrong mtdparts: %s\n", mtdparts);
+			return CMD_RET_FAILURE;
+		}
+
+		mtdname_len = colon - mtdparts;
+		strncpy(mtdname, mtdparts, mtdname_len);
+		mtdname[mtdname_len] = '\0';
+		/* Move the pointer forward (including the ':') */
+		mtdparts += mtdname_len + 1;
+		mtd = get_mtd_device_nm(mtdname);
+		if (IS_ERR_OR_NULL(mtd)) {
+			char altname[MTD_NAME_MAX_LEN];
+
+			/*
+			 * The MTD device named "mtdname" does not exist. Try to
+			 * find a correspondance with an MTD device having the
+			 * same type and number as defined in the mtdids.
+			 */
+			printf("No device named %s\n", mtdname);
+			ret = mtd_search_alternate_name(mtdname, altname,
+							MTD_NAME_MAX_LEN);
+			if (ret)  {
+				printf("Did not found an equivalent in mtdids\n");
+				return CMD_RET_FAILURE;
+			}
+
+			mtd = get_mtd_device_nm(altname);
+			if (IS_ERR_OR_NULL(mtd)) {
+				printf("No device named %s\n", altname);
+				return CMD_RET_FAILURE;
+			}
+		}
+		put_mtd_device(mtd);
+
+		/*
+		 * Parse the MTD device partitions. It will update the mtdparts
+		 * pointer, create an array of parts (that must be freed), and
+		 * return the number of partition structures in the array.
+		 */
+		ret = mtd_parse_partitions(mtd, &mtdparts, &parts, &len);
+		if (ret) {
+			printf("Could not parse device %s\n", mtd->name);
+			return CMD_RET_FAILURE;
+		}
+
+		if (!len)
+			continue;
+
+		/*
+		 * Create the new MTD partitions and free the structures
+		 * allocated during the parsing.
+		 */
+		add_mtd_partitions(mtd, parts, len);
+
+		for (i = 0; i < len; i++)
+			free(&parts[i].name);
+		free(parts);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
+{
+	do_div(len, mtd->writesize);
+
+	return len;
+}
+
+static bool mtd_is_aligned_with_min_io_size(struct mtd_info *mtd, u64 size)
+{
+	return do_div(size, mtd->writesize);
+}
+
+static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
+{
+	return do_div(size, mtd->erasesize);
+}
+
+static int do_mtd_list(void)
+{
+	struct mtd_info *mtd;
+	int dev_nb = 0;
+
+#if IS_ENABLED(CONFIG_MTD)
+	/* Ensure all devices (and their partitions) are probed */
+	mtd_probe_devices();
+#endif
+
+	printf("List of MTD devices:\n");
+	mtd_for_each_device(mtd) {
+		if (!mtd_is_partition(mtd))
+			mtd_show_device(mtd);
+
+		dev_nb++;
+	}
+
+	if (!dev_nb) {
+		printf("No MTD device found\n");
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct mtd_info *mtd;
+	const char *cmd;
+	char *mtdname;
+	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@least a device ID.
+	 * Check the selected device is valid. Ensure it is probed.
+	 */
+	if (argc < 3)
+		return CMD_RET_USAGE;
+
+	mtdname = argv[2];
+	mtd_probe_devices();
+	mtd = get_mtd_device_nm(mtdname);
+	if (IS_ERR_OR_NULL(mtd)) {
+		printf("MTD device %s not found, ret %ld\n",
+		       mtdname, PTR_ERR(mtd));
+		return CMD_RET_FAILURE;
+	}
+	put_mtd_device(mtd);
+
+	argc -= 3;
+	argv += 3;
+
+	/* Do the parsing */
+	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "dump", 4) ||
+	    !strncmp(cmd, "write", 5)) {
+		struct mtd_oob_ops io_op = {};
+		bool dump, read, raw, woob;
+		uint user_addr = 0;
+		uint nb_pages;
+		u8 *buf;
+		u64 off, len, default_len;
+		u32 oob_len;
+
+		dump = !strncmp(cmd, "dump", 4);
+		read = dump || !strncmp(cmd, "read", 4);
+		raw = strstr(cmd, ".raw");
+		woob = strstr(cmd, ".oob");
+
+		if (!dump) {
+			if (!argc)
+				return CMD_RET_USAGE;
+
+			user_addr = simple_strtoul(argv[0], NULL, 16);
+			argc--;
+			argv++;
+		}
+
+		default_len = dump ? mtd->writesize : mtd->size;
+		off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
+		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) :
+				 default_len;
+		nb_pages = mtd_len_to_pages(mtd, len);
+		oob_len = woob ? nb_pages * mtd->oobsize : 0;
+
+		if (mtd_is_aligned_with_min_io_size(mtd, off)) {
+			printf("Offset not aligned with a page (0x%x)\n",
+			       mtd->writesize);
+			return CMD_RET_FAILURE;
+		}
+
+		if (mtd_is_aligned_with_min_io_size(mtd, len)) {
+			printf("Size not a multiple of a page (0x%x)\n",
+			       mtd->writesize);
+			return CMD_RET_FAILURE;
+		}
+
+		if (dump)
+			buf = kmalloc(len + oob_len, GFP_KERNEL);
+		else
+			buf = map_sysmem(user_addr, 0);
+
+		if (!buf) {
+			printf("Could not map/allocate the user buffer\n");
+			return CMD_RET_FAILURE;
+		}
+
+		printf("%s %lldB (%d page(s)) at offset 0x%08llx%s%s\n",
+		       read ? "Reading" : "Writing", len, nb_pages, off,
+		       raw ? " [raw]" : "", woob ? " [oob]" : "");
+
+		io_op.mode = raw ? MTD_OPS_RAW : MTD_OPS_AUTO_OOB;
+		io_op.len = len;
+		io_op.ooblen = oob_len;
+		io_op.datbuf = buf;
+		io_op.oobbuf = woob ? &buf[len] : NULL;
+
+		if (read)
+			ret = mtd_read_oob(mtd, off, &io_op);
+		else
+			ret = mtd_write_oob(mtd, off, &io_op);
+
+		if (!ret && dump) {
+			uint page;
+
+			for (page = 0; page < nb_pages; page++) {
+				u64 data_off = page * mtd->writesize;
+
+				printf("\nDump %d data bytes from 0x%08llx:\n",
+				       mtd->writesize, data_off);
+				mtd_dump_buf(buf, mtd->writesize, data_off);
+
+				if (woob) {
+					u64 oob_off = page * mtd->oobsize;
+
+					printf("Dump %d OOB bytes from page at 0x%08llx:\n",
+					       mtd->oobsize, data_off);
+					mtd_dump_buf(&buf[len + oob_off],
+						     mtd->oobsize, 0);
+				}
+			}
+		}
+
+		if (dump)
+			kfree(buf);
+		else
+			unmap_sysmem(buf);
+
+		if (ret) {
+			printf("%s on %s failed with error %d\n",
+			       read ? "Read" : "Write", mtd->name, ret);
+			return CMD_RET_FAILURE;
+		}
+
+	} else if (!strcmp(cmd, "erase")) {
+		bool scrub = strstr(cmd, ".dontskipbad");
+		struct erase_info erase_op = {};
+		u64 off, len;
+
+		off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
+		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->size;
+
+		if (mtd_is_aligned_with_block_size(mtd, off)) {
+			printf("Offset not aligned with a block (0x%x)\n",
+			       mtd->erasesize);
+			return CMD_RET_FAILURE;
+		}
+
+		if (mtd_is_aligned_with_block_size(mtd, len)) {
+			printf("Size not a multiple of a block (0x%x)\n",
+			       mtd->erasesize);
+			return CMD_RET_FAILURE;
+		}
+
+		printf("Erasing 0x%08llx ... 0x%08llx (%d eraseblock(s))\n",
+		       off, off + len - 1, mtd_div_by_eb(len, mtd));
+
+		erase_op.mtd = mtd;
+		erase_op.addr = off;
+		erase_op.len = len;
+		erase_op.scrub = scrub;
+
+		while (erase_op.len) {
+			ret = mtd_erase(mtd, &erase_op);
+
+			/* Abort if its not an bad block error */
+			if (ret != -EIO)
+				break;
+
+			printf("Skipping bad block at 0x%08llx\n",
+			       erase_op.fail_addr);
+
+			/* Skip bad block and continue behind it */
+			erase_op.len -= erase_op.fail_addr - erase_op.addr;
+			erase_op.len -= mtd->erasesize;
+			erase_op.addr = erase_op.fail_addr + mtd->erasesize;
+		}
+	} else {
+		return CMD_RET_USAGE;
+	}
+
+	if (ret)
+		return CMD_RET_FAILURE;
+
+	return CMD_RET_SUCCESS;
+}
+
+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> <addr> [<off> [<size>]]\n"
+	"mtd dump[.raw][.oob]    <name>        [<off> [<size>]]\n"
+	"mtd write[.raw][.oob]   <name> <addr> [<off> [<size>]]\n"
+	"mtd erase[.dontskipbad] <name>        [<off> [<size>]]\n"
+	"\n"
+	"With:\n"
+	"\t<name>: NAND partition/chip name\n"
+	"\t<addr>: user address from/to which data will be retrieved/stored\n"
+	"\t<off>: offset in <name> in bytes (default: start of the part)\n"
+	"\t\t* must be block-aligned for erase\n"
+	"\t\t* must be page-aligned otherwise\n"
+	"\t<size>: length of the operation in bytes (default: the entire device)\n"
+	"\t\t* must be a multiple of a block for erase\n"
+	"\t\t* must be a multiple of a page otherwise (special case: default is a page with dump)\n"
+#endif
+	"";
+
+U_BOOT_CMD(mtd, 10, 1, do_mtd, "MTD utils", mtd_help_text);
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index cdf515256a..22ceda93c0 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -3,7 +3,7 @@
 # (C) Copyright 2000-2007
 # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
 
-ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND)$(CONFIG_CMD_SF)))
+ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND)$(CONFIG_CMD_SF)$(CONFIG_CMD_MTD)))
 obj-y += mtdcore.o mtd_uboot.o
 endif
 obj-$(CONFIG_MTD) += mtd-uclass.o
diff --git a/include/mtd.h b/include/mtd.h
index 6e6da3002f..04b1c50ac7 100644
--- a/include/mtd.h
+++ b/include/mtd.h
@@ -20,5 +20,6 @@ static inline struct mtd_info *mtd_get_info(struct udevice *dev)
 }
 
 int mtd_probe(struct udevice *dev);
+int mtd_probe_devices(void);
 
 #endif	/* _MTD_H_ */
-- 
2.17.1

[U-Boot] [PATCH v7 11/13] cmd: mtdparts: try to probe the MTD devices as a fallback

[Miquel Raynal]

Current implementation of mtdparts command errors out if the desired MTD
device is not found. Fallback to the new probe function in this case
before erroring out.

This will the save the user the need to call something like 'mtd list'
before mtdparts.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jagan Teki <jagan@openedev.com>
---
 cmd/mtdparts.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index f7ed1a0779..a1102c3fc5 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -79,6 +79,10 @@
 #include <linux/err.h>
 #include <linux/mtd/mtd.h>
 
+#if defined(CONFIG_MTD)
+#include <mtd.h>
+#endif
+
 #if defined(CONFIG_CMD_NAND)
 #include <linux/mtd/rawnand.h>
 #include <nand.h>
@@ -307,9 +311,15 @@ static int get_mtd_info(u8 type, u8 num, struct mtd_info **mtd)
 
 	sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(type), num);
 	*mtd = get_mtd_device_nm(mtd_dev);
-	if (IS_ERR(*mtd)) {
-		printf("Device %s not found!\n", mtd_dev);
-		return 1;
+	if (IS_ERR_OR_NULL(*mtd)) {
+#ifdef CONFIG_MTD
+		mtd_probe_devices();
+		*mtd = get_mtd_device_nm(mtd_dev);
+#endif
+		if (IS_ERR_OR_NULL(*mtd)) {
+			printf("Device %s not found!\n", mtd_dev);
+			return 1;
+		}
 	}
 	put_mtd_device(*mtd);
 
-- 
2.17.1

[U-Boot] [PATCH v7 12/13] cmd: ubi: clean the partition handling

[Miquel Raynal]

UBI should not mess with MTD partitions, now that the partitions are
handled in a clean way, clean the ubi command and avoid using this
uneeded extra-glue to reference the devices.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 cmd/Kconfig |   2 ++
 cmd/ubi.c   | 100 +++++++++++++++-------------------------------------
 2 files changed, 31 insertions(+), 71 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 0778d2ecff..4deec0b238 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1815,6 +1815,8 @@ config CMD_UBI
 	  capabilities. Please, consult the MTD web site for more details
 	  (www.linux-mtd.infradead.org). Activate this option if you want
 	  to use U-Boot UBI commands.
+	  It is also strongly encouraged to also enable CONFIG_MTD to get full
+	  partition support.
 
 config CMD_UBIFS
 	tristate "Enable UBIFS - Unsorted block images filesystem commands"
diff --git a/cmd/ubi.c b/cmd/ubi.c
index 0a3405a3b1..af0cea2ca5 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -15,6 +15,9 @@
 #include <command.h>
 #include <exports.h>
 #include <memalign.h>
+#if defined(CONFIG_MTD)
+#include <mtd.h>
+#endif
 #include <nand.h>
 #include <onenand_uboot.h>
 #include <linux/mtd/mtd.h>
@@ -29,17 +32,6 @@
 
 /* Private own data */
 static struct ubi_device *ubi;
-static char buffer[80];
-static int ubi_initialized;
-
-struct selected_dev {
-	char part_name[80];
-	int selected;
-	int nr;
-	struct mtd_info *mtd_info;
-};
-
-static struct selected_dev ubi_dev;
 
 #ifdef CONFIG_CMD_UBIFS
 int ubifs_is_mounted(void);
@@ -404,43 +396,24 @@ int ubi_volume_read(char *volume, char *buf, size_t size)
 	return err;
 }
 
-static int ubi_dev_scan(struct mtd_info *info, char *ubidev,
-		const char *vid_header_offset)
+static int ubi_dev_scan(struct mtd_info *info, const char *vid_header_offset)
 {
-	struct mtd_device *dev;
-	struct part_info *part;
-	struct mtd_partition mtd_part;
 	char ubi_mtd_param_buffer[80];
-	u8 pnum;
 	int err;
 
-	if (find_dev_and_part(ubidev, &dev, &pnum, &part) != 0)
-		return 1;
+	if (!vid_header_offset)
+		sprintf(ubi_mtd_param_buffer, "%s", info->name);
+	else
+		sprintf(ubi_mtd_param_buffer, "%s,%s", info->name,
+			vid_header_offset);
 
-	sprintf(buffer, "mtd=%d", pnum);
-	memset(&mtd_part, 0, sizeof(mtd_part));
-	mtd_part.name = buffer;
-	mtd_part.size = part->size;
-	mtd_part.offset = part->offset;
-	add_mtd_partitions(info, &mtd_part, 1);
-
-	strcpy(ubi_mtd_param_buffer, buffer);
-	if (vid_header_offset)
-		sprintf(ubi_mtd_param_buffer, "mtd=%d,%s", pnum,
-				vid_header_offset);
 	err = ubi_mtd_param_parse(ubi_mtd_param_buffer, NULL);
-	if (err) {
-		del_mtd_partitions(info);
+	if (err)
 		return -err;
-	}
 
 	err = ubi_init();
-	if (err) {
-		del_mtd_partitions(info);
+	if (err)
 		return -err;
-	}
-
-	ubi_initialized = 1;
 
 	return 0;
 }
@@ -465,50 +438,35 @@ int ubi_detach(void)
 	/*
 	 * Call ubi_exit() before re-initializing the UBI subsystem
 	 */
-	if (ubi_initialized) {
+	if (ubi)
 		ubi_exit();
-		del_mtd_partitions(ubi_dev.mtd_info);
-		ubi_initialized = 0;
-	}
 
-	ubi_dev.selected = 0;
+	ubi = NULL;
+
 	return 0;
 }
 
 int ubi_part(char *part_name, const char *vid_header_offset)
 {
+	struct mtd_info *mtd;
 	int err = 0;
-	char mtd_dev[16];
-	struct mtd_device *dev;
-	struct part_info *part;
-	u8 pnum;
 
 	ubi_detach();
-	/*
-	 * Search the mtd device number where this partition
-	 * is located
-	 */
-	if (find_dev_and_part(part_name, &dev, &pnum, &part)) {
+
+#ifdef CONFIG_MTD
+	mtd_probe_devices();
+#endif
+	mtd = get_mtd_device_nm(part_name);
+	if (IS_ERR(mtd)) {
 		printf("Partition %s not found!\n", part_name);
 		return 1;
 	}
-	sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(dev->id->type), dev->id->num);
-	ubi_dev.mtd_info = get_mtd_device_nm(mtd_dev);
-	if (IS_ERR(ubi_dev.mtd_info)) {
-		printf("Partition %s not found on device %s!\n", part_name,
-		       mtd_dev);
-		return 1;
-	}
+	put_mtd_device(mtd);
 
-	ubi_dev.selected = 1;
-
-	strcpy(ubi_dev.part_name, part_name);
-	err = ubi_dev_scan(ubi_dev.mtd_info, ubi_dev.part_name,
-			vid_header_offset);
+	err = ubi_dev_scan(mtd, vid_header_offset);
 	if (err) {
 		printf("UBI init error %d\n", err);
 		printf("Please check, if the correct MTD partition is used (size big enough?)\n");
-		ubi_dev.selected = 0;
 		return err;
 	}
 
@@ -539,13 +497,13 @@ static int do_ubi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 		/* Print current partition */
 		if (argc == 2) {
-			if (!ubi_dev.selected) {
-				printf("Error, no UBI device/partition selected!\n");
+			if (!ubi) {
+				printf("Error, no UBI device selected!\n");
 				return 1;
 			}
 
-			printf("Device %d: %s, partition %s\n",
-			       ubi_dev.nr, ubi_dev.mtd_info->name, ubi_dev.part_name);
+			printf("Device %d: %s, MTD partition %s\n",
+			       ubi->ubi_num, ubi->ubi_name, ubi->mtd->name);
 			return 0;
 		}
 
@@ -558,8 +516,8 @@ static int do_ubi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return ubi_part(argv[2], vid_header_offset);
 	}
 
-	if ((strcmp(argv[1], "part") != 0) && (!ubi_dev.selected)) {
-		printf("Error, no UBI device/partition selected!\n");
+	if ((strcmp(argv[1], "part") != 0) && !ubi) {
+		printf("Error, no UBI device selected!\n");
 		return 1;
 	}
 
-- 
2.17.1

[U-Boot] [PATCH v7 13/13] cmd: mtdparts: describe as legacy

[Miquel Raynal]

The 'mtdparts' command is not needed anymore. While the environment
variable is still valid (and useful), the command has been replaced by
'mtd' which is much more close to the MTD stack and do not add its own
specific glue. The 'mtdids' variable, only used by the 'mtdparts'
command is also useless if the right MTD device name is used in the
'mtdparts' variable.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 cmd/Kconfig | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 4deec0b238..0786663f4a 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1673,7 +1673,11 @@ config CMD_MTDPARTS
 	bool "MTD partition support"
 	select MTD_DEVICE if (CMD_NAND || NAND)
 	help
-	  MTD partition support
+	  MTD partitioning tool support.
+	  It is strongly encouraged to avoid using this command
+	  anymore. One can still declare the partitions in the
+	  mtdparts environment variable but better use the MTD stack
+	  and the mtd command instead than this one.
 
 config MTDIDS_DEFAULT
 	string "Default MTD IDs"
@@ -1681,6 +1685,10 @@ config MTDIDS_DEFAULT
 	help
 	  Defines a default MTD IDs list for use with MTD partitions in the
 	  Linux MTD command line partitions format.
+	  Declaration of this environment variable is not useful
+	  anymore when using the right MTD names in mtdparts along
+	  with the use of the 'mtd' command instead of the legacy
+	  'mtdparts'.
 
 config MTDPARTS_DEFAULT
 	string "Default MTD partition scheme"
-- 
2.17.1

[U-Boot] [PATCH v7 01/13] lib: strto: parse all lowercase metric prefixes in ustrtoul[l]

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> Both ustrtoul and ustrtoull interpret 1k but not 1m or 1g. Even if the
> SI symbols for Mega and Giga are 'M' and 'G', certain entries of
> eg. mtdparts also use (wrongly) the metric prefix 'm' and 'g'.
> 
> I do not see how parsing lowercase prefixes could break anything, so
> parse them like their uppercase counterpart.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   lib/strto.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/lib/strto.c b/lib/strto.c
> index 7f6076909a..84f8d92d57 100644
> --- a/lib/strto.c
> +++ b/lib/strto.c
> @@ -87,9 +87,11 @@ unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
>   	unsigned long result = simple_strtoul(cp, endp, base);
>   	switch (**endp) {
>   	case 'G':
> +	case 'g':
>   		result *= 1024;
>   		/* fall through */
>   	case 'M':
> +	case 'm':
>   		result *= 1024;
>   		/* fall through */
>   	case 'K':
> @@ -110,9 +112,11 @@ unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
>   	unsigned long long result = simple_strtoull(cp, endp, base);
>   	switch (**endp) {
>   	case 'G':
> +	case 'g':
>   		result *= 1024;
>   		/* fall through */
>   	case 'M':
> +	case 'm':

Wouldn't it be better, to use tolower() on the char and drop all the
upper case checks completely - also for the 'K' case?

Thanks,
Stefan

[U-Boot] [PATCH v7 02/13] lib: strto: fix metric suffix parsing in strtoul[l]

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> While 1kB or 1kiB will be parsed correctly, 1k will return the right
> amount, but the metric suffix will not be escaped once the char
> pointer updated. Fix this situation by simplifying the move of the
> endp pointer.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   lib/strto.c | 22 ++++++++++------------
>   1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/strto.c b/lib/strto.c
> index 84f8d92d57..502a0153e7 100644
> --- a/lib/strto.c
> +++ b/lib/strto.c
> @@ -97,12 +97,11 @@ unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
>   	case 'K':
>   	case 'k':
>   		result *= 1024;
> -		if ((*endp)[1] == 'i') {
> -			if ((*endp)[2] == 'B')
> -				(*endp) += 3;
> -			else
> -				(*endp) += 2;
> -		}
> +		(*endp)++;
> +		if (**endp == 'i')
> +			(*endp)++;
> +		if (**endp == 'B')
> +			(*endp)++;
>   	}
>   	return result;
>   }
> @@ -122,12 +121,11 @@ unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
>   	case 'K':
>   	case 'k':
>   		result *= 1024;
> -		if ((*endp)[1] == 'i') {
> -			if ((*endp)[2] == 'B')
> -				(*endp) += 3;
> -			else
> -				(*endp) += 2;
> -		}
> +		(*endp)++;
> +		if (**endp == 'i')
> +			(*endp)++;
> +		if (**endp == 'B')
> +			(*endp)++;
>   	}
>   	return result;
>   }
> 

Even though KiB is not equal to KB in general (at least in Linux
userspace AFAIK), lets not change this in U-Boot and always use
KiB and KB as a representation for 1024 (instead of 1000). So:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 03/13] mtd: Kconfig: remove MTD_PARTITIONS duplicated symbol

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> MTD_PARTITIONS is declared twice. Remove the redundant entry with no
> help associated.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   drivers/mtd/Kconfig | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index d98457e223..9341d518f3 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -1,8 +1,5 @@
>   menu "MTD Support"
>   
> -config MTD_PARTITIONS
> -	bool
> -
>   config MTD
>   	bool "Enable Driver Model for MTD drivers"
>   	depends on DM
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 04/13] cmd: mtdparts: accept spi-nand devices

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> Let spi-nand devices be recognized by mtdparts. This is superfluous
> but a full mtdparts rework would be very time-consuming.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Jagan Teki <jagan@openedev.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>   cmd/mtdparts.c              | 13 ++++++++-----
>   include/jffs2/load_kernel.h |  7 +++++--
>   2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
> index 756fc6018f..2e547894c6 100644
> --- a/cmd/mtdparts.c
> +++ b/cmd/mtdparts.c
> @@ -37,7 +37,7 @@
>    * mtdids=<idmap>[,<idmap>,...]
>    *
>    * <idmap>    := <dev-id>=<mtd-id>
> - * <dev-id>   := 'nand'|'nor'|'onenand'<dev-num>
> + * <dev-id>   := 'nand'|'nor'|'onenand'|'spi-nand'<dev-num>
>    * <dev-num>  := mtd device number, 0...
>    * <mtd-id>   := unique device tag used by linux kernel to find mtd device (mtd->name)
>    *
> @@ -339,7 +339,7 @@ static int part_validate_eraseblock(struct mtdids *id, struct part_info *part)
>   
>   	if (!mtd->numeraseregions) {
>   		/*
> -		 * Only one eraseregion (NAND, OneNAND or uniform NOR),
> +		 * Only one eraseregion (NAND, SPI-NAND, OneNAND or uniform NOR),
>   		 * checking for alignment is easy here
>   		 */
>   		offset = part->offset;
> @@ -1030,7 +1030,7 @@ static struct mtdids* id_find_by_mtd_id(const char *mtd_id, unsigned int mtd_id_
>   }
>   
>   /**
> - * Parse device id string <dev-id> := 'nand'|'nor'|'onenand'<dev-num>,
> + * Parse device id string <dev-id> := 'nand'|'nor'|'onenand'|'spi-nand'<dev-num>,
>    * return device type and number.
>    *
>    * @param id string describing device id
> @@ -1054,6 +1054,9 @@ int mtd_id_parse(const char *id, const char **ret_id, u8 *dev_type,
>   	} else if (strncmp(p, "onenand", 7) == 0) {
>   		*dev_type = MTD_DEV_TYPE_ONENAND;
>   		p += 7;
> +	} else if (strncmp(p, "spi-nand", 8) == 0) {
> +		*dev_type = MTD_DEV_TYPE_SPINAND;
> +		p += 8;
>   	} else {
>   		printf("incorrect device type in %s\n", id);
>   		return 1;
> @@ -1636,7 +1639,7 @@ static int parse_mtdids(const char *const ids)
>   	while(p && (*p != '\0')) {
>   
>   		ret = 1;
> -		/* parse 'nor'|'nand'|'onenand'<dev-num> */
> +		/* parse 'nor'|'nand'|'onenand'|'spi-nand'<dev-num> */
>   		if (mtd_id_parse(p, &p, &type, &num) != 0)
>   			break;
>   
> @@ -2112,7 +2115,7 @@ static char mtdparts_help_text[] =
>   	"'mtdids' - linux kernel mtd device id <-> u-boot device id mapping\n\n"
>   	"mtdids=<idmap>[,<idmap>,...]\n\n"
>   	"<idmap>    := <dev-id>=<mtd-id>\n"
> -	"<dev-id>   := 'nand'|'nor'|'onenand'<dev-num>\n"
> +	"<dev-id>   := 'nand'|'nor'|'onenand'|'spi-nand'<dev-num>\n"
>   	"<dev-num>  := mtd device number, 0...\n"
>   	"<mtd-id>   := unique device tag used by linux kernel to find mtd device (mtd->name)\n\n"
>   	"'mtdparts' - partition list\n\n"
> diff --git a/include/jffs2/load_kernel.h b/include/jffs2/load_kernel.h
> index 1ddff062ad..9346d7ee9f 100644
> --- a/include/jffs2/load_kernel.h
> +++ b/include/jffs2/load_kernel.h
> @@ -15,9 +15,12 @@
>   #define MTD_DEV_TYPE_NOR	0x0001
>   #define MTD_DEV_TYPE_NAND	0x0002
>   #define MTD_DEV_TYPE_ONENAND	0x0004
> +#define MTD_DEV_TYPE_SPINAND	0x0008
>   
> -#define MTD_DEV_TYPE(type) ((type == MTD_DEV_TYPE_NAND) ? "nand" :	\
> -			(type == MTD_DEV_TYPE_ONENAND) ? "onenand" : "nor")
> +#define MTD_DEV_TYPE(type) (type == MTD_DEV_TYPE_NAND ? "nand" :	\
> +			    (type == MTD_DEV_TYPE_NOR ? "nor" :		\
> +			     (type == MTD_DEV_TYPE_ONENAND ? "onenand" : \
> +			      "spi-nand")))				\
>   
>   struct mtd_device {
>   	struct list_head link;
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 05/13] cmd: mtdparts: remove mandatory 'mtdparts=' prefix

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> All U-Boot users must define the mtdparts environment variable with:
> setenv mtdparts mtdparts=...
> 
> While this may ease the partition declaration job to be passed to
> Linux, this is a pure software limitation and forcing this prefix is a
> complete non-sense. Let the user to declare manually the mtdparts
> variable without the prefix.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Jagan Teki <jagan@openedev.com>
> ---
>   cmd/mtdparts.c | 17 ++++++-----------
>   1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
> index 2e547894c6..f7ed1a0779 100644
> --- a/cmd/mtdparts.c
> +++ b/cmd/mtdparts.c
> @@ -44,7 +44,7 @@
>    *
>    * 'mtdparts' - partition list
>    *
> - * mtdparts=mtdparts=<mtd-def>[;<mtd-def>...]
> + * mtdparts=[mtdparts=]<mtd-def>[;<mtd-def>...]
>    *
>    * <mtd-def>  := <mtd-id>:<part-def>[,<part-def>...]
>    * <mtd-id>   := unique device tag used by linux kernel to find mtd device (mtd->name)
> @@ -62,11 +62,11 @@
>    *
>    * 1 NOR Flash, with 1 single writable partition:
>    * mtdids=nor0=edb7312-nor
> - * mtdparts=mtdparts=edb7312-nor:-
> + * mtdparts=[mtdparts=]edb7312-nor:-
>    *
>    * 1 NOR Flash with 2 partitions, 1 NAND with one
>    * mtdids=nor0=edb7312-nor,nand0=edb7312-nand
> - * mtdparts=mtdparts=edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
> + * mtdparts=[mtdparts=]edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
>    *
>    */
>   
> @@ -1099,9 +1099,6 @@ static int generate_mtdparts(char *buf, u32 buflen)
>   		return 0;
>   	}
>   
> -	strcpy(p, "mtdparts=");
> -	p += 9;
> -
>   	list_for_each(dentry, &devices) {
>   		dev = list_entry(dentry, struct mtd_device, link);
>   
> @@ -1572,11 +1569,9 @@ static int parse_mtdparts(const char *const mtdparts)
>   	if (!p)
>   		p = mtdparts;
>   
> -	if (strncmp(p, "mtdparts=", 9) != 0) {
> -		printf("mtdparts variable doesn't start with 'mtdparts='\n");
> -		return err;
> -	}
> -	p += 9;
> +	/* Skip the useless prefix, if any */
> +	if (strncmp(p, "mtdparts=", 9) == 0)
> +		p += 9;
>   
>   	while (*p != '\0') {
>   		err = 1;
> 

I always wondered about this double mtdparts thing but was never brave
enough to touch it. ;)

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 06/13] mtd: uclass: add probe function

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> The user might want to trigger the probe of any MTD device, export these
> functions so they can be called from a command source file.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Jagan Teki <jagan@openedev.com>
> ---
>   drivers/mtd/mtd-uclass.c | 16 ++++++++++++++++
>   include/mtd.h            |  2 ++
>   2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/mtd/mtd-uclass.c b/drivers/mtd/mtd-uclass.c
> index 9ca049c437..5418217431 100644
> --- a/drivers/mtd/mtd-uclass.c
> +++ b/drivers/mtd/mtd-uclass.c
> @@ -5,9 +5,25 @@
>   
>   #include <common.h>
>   #include <dm.h>
> +#include <dm/device-internal.h>
>   #include <errno.h>
>   #include <mtd.h>
>   
> +/**
> + * mtd_probe - Probe the device @dev if not already done
> + *
> + * @dev: U-Boot device to probe
> + *
> + * @return 0 on success, an error otherwise.
> + */
> +int mtd_probe(struct udevice *dev)
> +{
> +	if (device_active(dev))
> +		return 0;
> +
> +	return device_probe(dev);
> +}
> +
>   /*
>    * Implement a MTD uclass which should include most flash drivers.
>    * The uclass private is pointed to mtd_info.
> diff --git a/include/mtd.h b/include/mtd.h
> index 548e7f191b..6e6da3002f 100644
> --- a/include/mtd.h
> +++ b/include/mtd.h
> @@ -19,4 +19,6 @@ static inline struct mtd_info *mtd_get_info(struct udevice *dev)
>   	return dev_get_uclass_priv(dev);
>   }
>   
> +int mtd_probe(struct udevice *dev);
> +
>   #endif	/* _MTD_H_ */
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 07/13] mtd: uclass: add a generic 'mtdparts' parser

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> The current parser is very specific to U-Boot mtdparts implementation.
> It does not use MTD structures like mtd_info and mtd_partition. Copy
> and adapt the current parser in drivers/mtd/mtd-uclass.c (to not break
> the current use of mtdparts.c itself) and write some kind of a wrapper
> around the current implementation to allow other commands to benefit
> from this parsing in a user-friendly way.
> 
> This new function will allocate an mtd_partition array for each
> successful call. This array must be freed after use by the caller.
> The given 'mtdparts' buffer pointer will be moved forward to the next
> MTD device (if any, it will point towards a '\0' character otherwise).
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Jagan Teki <jagan@openedev.com>
> ---
>   drivers/mtd/mtdpart.c          | 187 +++++++++++++++++++++++++++++++++
>   include/linux/mtd/partitions.h |   2 +
>   2 files changed, 189 insertions(+)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 9ccb1b3361..d9708da2ae 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -76,6 +76,193 @@ char *kstrdup(const char *s, gfp_t gfp)
>   }
>   #endif
>   
> +#define MTD_SIZE_REMAINING		(~0LLU)
> +#define MTD_OFFSET_NOT_SPECIFIED	(~0LLU)
> +
> +/**
> + * mtd_parse_partition - Parse @mtdparts partition definition, fill @partition
> + *                       with it and update the @mtdparts string pointer.
> + *
> + * The partition name is allocated and must be freed by the caller.
> + *
> + * This function is widely inspired from part_parse (mtdparts.c).
> + *
> + * @mtdparts: String describing the partition with mtdparts command syntax
> + * @partition: MTD partition structure to fill
> + *
> + * @return 0 on success, an error otherwise.
> + */
> +static int mtd_parse_partition(const char **_mtdparts,
> +			       struct mtd_partition *partition)
> +{
> +	const char *mtdparts = *_mtdparts;
> +	const char *name = NULL;
> +	int name_len;
> +	char *buf;
> +
> +	/* Ensure the partition structure is empty */
> +	memset(partition, 0, sizeof(struct mtd_partition));
> +
> +	/* Fetch the partition size */
> +	if (*mtdparts == '-') {
> +		/* Assign all remaining space to this partition */
> +		partition->size = MTD_SIZE_REMAINING;
> +		mtdparts++;
> +	} else {
> +		partition->size = ustrtoull(mtdparts, (char **)&mtdparts, 0);
> +		if (partition->size < SZ_4K) {
> +			printf("Minimum allowed partition size is 4kiB\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Check for the offset */
> +	partition->offset = MTD_OFFSET_NOT_SPECIFIED;
> +	if (*mtdparts == '@') {
> +		mtdparts++;
> +		partition->offset = ustrtoull(mtdparts, (char **)&mtdparts, 0);
> +	}
> +
> +	/* Now look for the name */
> +	if (*mtdparts == '(') {
> +		name = ++mtdparts;
> +		if ((mtdparts = strchr(name, ')')) == NULL) {
> +			printf("No closing ')' found in partition name\n");
> +			return -EINVAL;
> +		}
> +		name_len = mtdparts - name + 1;
> +		if ((name_len - 1) == 0) {
> +			printf("Empty partition name\n");
> +			return -EINVAL;
> +		}
> +		mtdparts++;
> +	} else {
> +		/* Name will be of the form size at offset */
> +		name_len = 22;
> +	}
> +
> +	/* Check if the partition is read-only */
> +	if (strncmp(mtdparts, "ro", 2) == 0) {
> +		partition->mask_flags |= MTD_WRITEABLE;
> +		mtdparts += 2;
> +	}
> +
> +	/* Check for a potential next partition definition */
> +	if (*mtdparts == ',') {
> +		if (partition->size == MTD_SIZE_REMAINING) {
> +			printf("No partitions allowed after a fill-up partition\n");
> +			return -EINVAL;
> +		}
> +		++mtdparts;
> +	} else if (*mtdparts == ';') {
> +		++mtdparts;
> +	} else if (*mtdparts == '\0') {
> +		/* NOP */
> +	} else {
> +		printf("Unexpected character '%c' in mtdparts\n", *mtdparts);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Allocate a buffer for the name and either copy the provided name or
> +	 * auto-generate it with the form 'size at offset'.
> +	 */
> +	buf = malloc(name_len);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (name)
> +		strncpy(buf, name, name_len - 1);
> +	else
> +		snprintf(buf, name_len, "0x%08llx at 0x%08llx",
> +			 partition->size, partition->offset);
> +
> +	buf[name_len - 1] = '\0';
> +	partition->name = buf;
> +
> +	*_mtdparts = mtdparts;
> +
> +	return 0;
> +}
> +
> +/**
> + * mtdparts_parse_part - Create a partition array from an mtdparts definition
> + *
> + * Stateless function that takes a @parent MTD device, a string @_mtdparts
> + * describing the partitions (with the "mtdparts" command syntax) and creates
> + * the corresponding MTD partition structure array @_parts. Both the name and
> + * the structure partition itself must be freed freed by the caller.
> + *
> + * @parent: MTD device which contains the partitions
> + * @_mtdparts: Pointer to a string describing the partitions with "mtdparts"
> + *             command syntax.
> + * @_parts: Allocated array containing the partitions, must be freed by the
> + *          caller.
> + * @_nb_parts: Size of @_parts array.
> + *
> + * @return 0 on success, an error otherwise.
> + */
> +int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
> +			 struct mtd_partition **_parts, int *_nb_parts)
> +{
> +	struct mtd_partition partition = {}, *parts;
> +	const char *mtdparts = *_mtdparts;
> +	int cur_off = 0, cur_sz = 0;
> +	int nb_parts = 0;
> +	int ret, idx;
> +	u64 sz;
> +
> +	/* First, iterate over the partitions until we know their number */
> +	while (mtdparts[0] != '\0' && mtdparts[0] != ';') {
> +		ret = mtd_parse_partition(&mtdparts, &partition);
> +		if (ret)
> +			return ret;
> +
> +		free((char *)partition.name);
> +		nb_parts++;
> +	}
> +
> +	/* Allocate an array of partitions to give back to the caller */
> +	parts = malloc(sizeof(*parts) * nb_parts);
> +	if (!parts) {
> +		printf("Could not allocate enough space to save partitions meta-data\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Iterate again over each partition to save the data in our array */
> +	for (idx = 0; idx < nb_parts; idx++) {
> +		ret = mtd_parse_partition(_mtdparts, &parts[idx]);
> +		if (ret)
> +			return ret;
> +
> +		if (parts[idx].size == MTD_SIZE_REMAINING)
> +			parts[idx].size = parent->size - cur_sz;
> +		cur_sz += parts[idx].size;
> +
> +		sz = parts[idx].size;
> +		if (sz < parent->writesize || do_div(sz, parent->writesize)) {
> +			printf("Partition size must be a multiple of %d\n",
> +			       parent->writesize);
> +			return -EINVAL;
> +		}
> +
> +		if (parts[idx].offset == MTD_OFFSET_NOT_SPECIFIED)
> +			parts[idx].offset = cur_off;
> +		cur_off += parts[idx].size;
> +
> +		parts[idx].ecclayout = parent->ecclayout;
> +	}
> +
> +	/* Offset by one mtdparts to point to the next device if any */
> +	if (*_mtdparts[0] == ';')
> +		_mtdparts++;
> +
> +	*_parts = parts;
> +	*_nb_parts = nb_parts;
> +
> +	return 0;
> +}
> +
>   /*
>    * MTD methods which simply translate the effective address and pass through
>    * to the _real_ device.
> diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
> index ce0e8dbee4..ed4ece5e13 100644
> --- a/include/linux/mtd/partitions.h
> +++ b/include/linux/mtd/partitions.h
> @@ -86,5 +86,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>   		      long long offset, long long length);
>   int mtd_del_partition(struct mtd_info *master, int partno);
>   uint64_t mtd_get_device_size(const struct mtd_info *mtd);
> +int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
> +			 struct mtd_partition **_parts, int *_nb_parts);
>   
>   #endif
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 08/13] mtd: uclass: search for an equivalent MTD name with the mtdids

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> Using an MTD device (resp. partition) name in mtdparts is simple and
> straightforward. However, for a long time already, another name was
> given in mtdparts to indicate a device (resp. partition) so the
> "mtdids" environment variable was created to do the match.
> 
> Let's create a function that, from an MTD device (resp. partition)
> name, search for the equivalent name in the "mtdparts" environment
> variable thanks to the "mtdids" string.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   drivers/mtd/mtdpart.c          | 62 ++++++++++++++++++++++++++++++++++
>   include/linux/mtd/partitions.h |  2 ++
>   2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index d9708da2ae..096351e48b 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -263,6 +263,68 @@ int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
>   	return 0;
>   }
>   
> +/**
> + * mtd_search_alternate_name - Search an alternate name for @mtdname thanks to
> + *                             the mtdids legacy environment variable.
> + *
> + * The mtdids string is a list of comma-separated 'dev_id=mtd_id' tupples.
> + * Check if one of the mtd_id matches mtdname, in this case save dev_id in
> + * altname.
> + *
> + * @mtdname: Current MTD device name
> + * @altname: Alternate name to return
> + * @max_len: Length of the alternate name buffer
> + *
> + * @return 0 on success, an error otherwise.
> + */
> +int mtd_search_alternate_name(const char *mtdname, char *altname,
> +			      unsigned int max_len)
> +{
> +	const char *mtdids, *equal, *comma, *dev_id, *mtd_id;
> +	int dev_id_len, mtd_id_len;
> +
> +	mtdids = env_get("mtdids");
> +	if (!mtdids)
> +		return -EINVAL;
> +
> +	do {
> +		/* Find the '=' sign */
> +		dev_id = mtdids;
> +		equal = strchr(dev_id, '=');
> +		if (!equal)
> +			break;
> +
> +		dev_id_len = equal - mtdids;
> +		mtd_id = equal + 1;
> +
> +		/* Find the end of the tupple */
> +		comma = strchr(mtdids, ',');
> +		if (comma)
> +			mtd_id_len = comma - equal;
> +		else
> +			mtd_id_len = &mtdids[strlen(mtdids)] - equal;
> +
> +		if (!dev_id_len || !mtd_id_len)
> +			return -EINVAL;
> +
> +		if (dev_id_len + 1 > max_len)
> +			continue;
> +
> +		/* Compare the name we search with the current mtd_id */
> +		if (!strncmp(mtdname, mtd_id, mtd_id_len)) {
> +			strncpy(altname, dev_id, dev_id_len);
> +			altname[dev_id_len] = 0;
> +
> +			return 0;
> +		}
> +
> +		/* Go to the next tupple */
> +		mtdids = comma + 1;
> +	} while (comma);
> +
> +	return -EINVAL;
> +}
> +
>   /*
>    * MTD methods which simply translate the effective address and pass through
>    * to the _real_ device.
> diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
> index ed4ece5e13..082a4966ea 100644
> --- a/include/linux/mtd/partitions.h
> +++ b/include/linux/mtd/partitions.h
> @@ -88,5 +88,7 @@ int mtd_del_partition(struct mtd_info *master, int partno);
>   uint64_t mtd_get_device_size(const struct mtd_info *mtd);
>   int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
>   			 struct mtd_partition **_parts, int *_nb_parts);
> +int mtd_search_alternate_name(const char *mtdname, char *altname,
> +			      unsigned int max_len);
>   
>   #endif
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 09/13] mtd: mtdpart: implement proper partition handling

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> Instead of collecting partitions in a flat list, create a hierarchy
> within the mtd_info structure: use a partitions list to keep track of
> the partitions of an MTD device (which might be itself a partition of
> another MTD device), a pointer to the parent device (NULL when the MTD
> device is the root one, not a partition).
> 
> By also saving directly in mtd_info the offset of the partition, we
> can get rid of the mtd_part structure.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   drivers/mtd/mtdcore.c          |   2 +
>   drivers/mtd/mtdpart.c          | 412 ++++++++++++++-------------------
>   include/linux/mtd/mtd.h        |  31 +++
>   include/linux/mtd/partitions.h |   1 -
>   4 files changed, 208 insertions(+), 238 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index fb1d68d5e2..fb6c779abb 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -426,6 +426,8 @@ int add_mtd_device(struct mtd_info *mtd)
>   	mtd->index = i;
>   	mtd->usecount = 0;
>   
> +	INIT_LIST_HEAD(&mtd->partitions);
> +
>   	/* default value if not set by driver */
>   	if (mtd->bitflip_threshold == 0)
>   		mtd->bitflip_threshold = mtd->ecc_strength;
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 096351e48b..afc02dcb24 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -29,29 +29,12 @@
>   
>   #include "mtdcore.h"
>   
> -/* Our partition linked list */
> -static LIST_HEAD(mtd_partitions);
>   #ifndef __UBOOT__
>   static DEFINE_MUTEX(mtd_partitions_mutex);
>   #else
>   DEFINE_MUTEX(mtd_partitions_mutex);
>   #endif
>   
> -/* Our partition node structure */
> -struct mtd_part {
> -	struct mtd_info mtd;
> -	struct mtd_info *master;
> -	uint64_t offset;
> -	struct list_head list;
> -};
> -
> -/*
> - * Given a pointer to the MTD object in the mtd_part structure, we can retrieve
> - * the pointer to that structure with this macro.
> - */
> -#define PART(x)  ((struct mtd_part *)(x))
> -
> -
>   #ifdef __UBOOT__
>   /* from mm/util.c */
>   
> @@ -333,19 +316,18 @@ int mtd_search_alternate_name(const char *mtdname, char *altname,
>   static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
>   		size_t *retlen, u_char *buf)
>   {
> -	struct mtd_part *part = PART(mtd);
>   	struct mtd_ecc_stats stats;
>   	int res;
>   
> -	stats = part->master->ecc_stats;
> -	res = part->master->_read(part->master, from + part->offset, len,
> -				  retlen, buf);
> +	stats = mtd->parent->ecc_stats;
> +	res = mtd->parent->_read(mtd->parent, from + mtd->offset, len,
> +				 retlen, buf);
>   	if (unlikely(mtd_is_eccerr(res)))
>   		mtd->ecc_stats.failed +=
> -			part->master->ecc_stats.failed - stats.failed;
> +			mtd->parent->ecc_stats.failed - stats.failed;
>   	else
>   		mtd->ecc_stats.corrected +=
> -			part->master->ecc_stats.corrected - stats.corrected;
> +			mtd->parent->ecc_stats.corrected - stats.corrected;
>   	return res;
>   }
>   
> @@ -353,17 +335,13 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
>   static int part_point(struct mtd_info *mtd, loff_t from, size_t len,
>   		size_t *retlen, void **virt, resource_size_t *phys)
>   {
> -	struct mtd_part *part = PART(mtd);
> -
> -	return part->master->_point(part->master, from + part->offset, len,
> -				    retlen, virt, phys);
> +	return mtd->parent->_point(mtd->parent, from + mtd->offset, len,
> +				   retlen, virt, phys);
>   }
>   
>   static int part_unpoint(struct mtd_info *mtd, loff_t from, size_t len)
>   {
> -	struct mtd_part *part = PART(mtd);
> -
> -	return part->master->_unpoint(part->master, from + part->offset, len);
> +	return mtd->parent->_unpoint(mtd->parent, from + mtd->offset, len);
>   }
>   #endif
>   
> @@ -372,17 +350,13 @@ static unsigned long part_get_unmapped_area(struct mtd_info *mtd,
>   					    unsigned long offset,
>   					    unsigned long flags)
>   {
> -	struct mtd_part *part = PART(mtd);
> -
> -	offset += part->offset;
> -	return part->master->_get_unmapped_area(part->master, len, offset,
> -						flags);
> +	offset += mtd->offset;
> +	return mtd->parent->_get_unmapped_area(mtd->parent, len, offset, flags);
>   }
>   
>   static int part_read_oob(struct mtd_info *mtd, loff_t from,
>   		struct mtd_oob_ops *ops)
>   {
> -	struct mtd_part *part = PART(mtd);
>   	int res;
>   
>   	if (from >= mtd->size)
> @@ -407,7 +381,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
>   			return -EINVAL;
>   	}
>   
> -	res = part->master->_read_oob(part->master, from + part->offset, ops);
> +	res = mtd->parent->_read_oob(mtd->parent, from + mtd->offset, ops);
>   	if (unlikely(res)) {
>   		if (mtd_is_bitflip(res))
>   			mtd->ecc_stats.corrected++;
> @@ -420,99 +394,87 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
>   static int part_read_user_prot_reg(struct mtd_info *mtd, loff_t from,
>   		size_t len, size_t *retlen, u_char *buf)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_read_user_prot_reg(part->master, from, len,
> -						 retlen, buf);
> +	return mtd->parent->_read_user_prot_reg(mtd->parent, from, len,
> +						retlen, buf);
>   }
>   
>   static int part_get_user_prot_info(struct mtd_info *mtd, size_t len,
>   				   size_t *retlen, struct otp_info *buf)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_get_user_prot_info(part->master, len, retlen,
> -						 buf);
> +	return mtd->parent->_get_user_prot_info(mtd->parent, len, retlen,
> +						buf);
>   }
>   
>   static int part_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
>   		size_t len, size_t *retlen, u_char *buf)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_read_fact_prot_reg(part->master, from, len,
> -						 retlen, buf);
> +	return mtd->parent->_read_fact_prot_reg(mtd->parent, from, len,
> +						retlen, buf);
>   }
>   
>   static int part_get_fact_prot_info(struct mtd_info *mtd, size_t len,
>   				   size_t *retlen, struct otp_info *buf)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_get_fact_prot_info(part->master, len, retlen,
> -						 buf);
> +	return mtd->parent->_get_fact_prot_info(mtd->parent, len, retlen,
> +						buf);
>   }
>   
>   static int part_write(struct mtd_info *mtd, loff_t to, size_t len,
>   		size_t *retlen, const u_char *buf)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_write(part->master, to + part->offset, len,
> -				    retlen, buf);
> +	return mtd->parent->_write(mtd->parent, to + mtd->offset, len,
> +				   retlen, buf);
>   }
>   
>   static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
>   		size_t *retlen, const u_char *buf)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_panic_write(part->master, to + part->offset, len,
> -					  retlen, buf);
> +	return mtd->parent->_panic_write(mtd->parent, to + mtd->offset, len,
> +					 retlen, buf);
>   }
>   
>   static int part_write_oob(struct mtd_info *mtd, loff_t to,
>   		struct mtd_oob_ops *ops)
>   {
> -	struct mtd_part *part = PART(mtd);
> -
>   	if (to >= mtd->size)
>   		return -EINVAL;
>   	if (ops->datbuf && to + ops->len > mtd->size)
>   		return -EINVAL;
> -	return part->master->_write_oob(part->master, to + part->offset, ops);
> +	return mtd->parent->_write_oob(mtd->parent, to + mtd->offset, ops);
>   }
>   
>   static int part_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
>   		size_t len, size_t *retlen, u_char *buf)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_write_user_prot_reg(part->master, from, len,
> -						  retlen, buf);
> +	return mtd->parent->_write_user_prot_reg(mtd->parent, from, len,
> +						 retlen, buf);
>   }
>   
>   static int part_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
>   		size_t len)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_lock_user_prot_reg(part->master, from, len);
> +	return mtd->parent->_lock_user_prot_reg(mtd->parent, from, len);
>   }
>   
>   #ifndef __UBOOT__
>   static int part_writev(struct mtd_info *mtd, const struct kvec *vecs,
>   		unsigned long count, loff_t to, size_t *retlen)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_writev(part->master, vecs, count,
> -				     to + part->offset, retlen);
> +	return mtd->parent->_writev(mtd->parent, vecs, count,
> +				    to + mtd->offset, retlen);
>   }
>   #endif
>   
>   static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
>   {
> -	struct mtd_part *part = PART(mtd);
>   	int ret;
>   
> -	instr->addr += part->offset;
> -	ret = part->master->_erase(part->master, instr);
> +	instr->addr += mtd->offset;
> +	ret = mtd->parent->_erase(mtd->parent, instr);
>   	if (ret) {
>   		if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
> -			instr->fail_addr -= part->offset;
> -		instr->addr -= part->offset;
> +			instr->fail_addr -= mtd->offset;
> +		instr->addr -= mtd->offset;
>   	}
>   	return ret;
>   }
> @@ -520,11 +482,9 @@ static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
>   void mtd_erase_callback(struct erase_info *instr)
>   {
>   	if (instr->mtd->_erase == part_erase) {
> -		struct mtd_part *part = PART(instr->mtd);
> -
>   		if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
> -			instr->fail_addr -= part->offset;
> -		instr->addr -= part->offset;
> +			instr->fail_addr -= instr->mtd->offset;
> +		instr->addr -= instr->mtd->offset;
>   	}
>   	if (instr->callback)
>   		instr->callback(instr);
> @@ -533,107 +493,112 @@ EXPORT_SYMBOL_GPL(mtd_erase_callback);
>   
>   static int part_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_lock(part->master, ofs + part->offset, len);
> +	return mtd->parent->_lock(mtd->parent, ofs + mtd->offset, len);
>   }
>   
>   static int part_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_unlock(part->master, ofs + part->offset, len);
> +	return mtd->parent->_unlock(mtd->parent, ofs + mtd->offset, len);
>   }
>   
>   static int part_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_is_locked(part->master, ofs + part->offset, len);
> +	return mtd->parent->_is_locked(mtd->parent, ofs + mtd->offset, len);
>   }
>   
>   static void part_sync(struct mtd_info *mtd)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	part->master->_sync(part->master);
> +	mtd->parent->_sync(mtd->parent);
>   }
>   
>   #ifndef __UBOOT__
>   static int part_suspend(struct mtd_info *mtd)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	return part->master->_suspend(part->master);
> +	return mtd->parent->_suspend(mtd->parent);
>   }
>   
>   static void part_resume(struct mtd_info *mtd)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	part->master->_resume(part->master);
> +	mtd->parent->_resume(mtd->parent);
>   }
>   #endif
>   
>   static int part_block_isreserved(struct mtd_info *mtd, loff_t ofs)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	ofs += part->offset;
> -	return part->master->_block_isreserved(part->master, ofs);
> +	ofs += mtd->offset;
> +	return mtd->parent->_block_isreserved(mtd->parent, ofs);
>   }
>   
>   static int part_block_isbad(struct mtd_info *mtd, loff_t ofs)
>   {
> -	struct mtd_part *part = PART(mtd);
> -	ofs += part->offset;
> -	return part->master->_block_isbad(part->master, ofs);
> +	ofs += mtd->offset;
> +	return mtd->parent->_block_isbad(mtd->parent, ofs);
>   }
>   
>   static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
>   {
> -	struct mtd_part *part = PART(mtd);
>   	int res;
>   
> -	ofs += part->offset;
> -	res = part->master->_block_markbad(part->master, ofs);
> +	ofs += mtd->offset;
> +	res = mtd->parent->_block_markbad(mtd->parent, ofs);
>   	if (!res)
>   		mtd->ecc_stats.badblocks++;
>   	return res;
>   }
>   
> -static inline void free_partition(struct mtd_part *p)
> +static inline void free_partition(struct mtd_info *p)
>   {
> -	kfree(p->mtd.name);
> +	kfree(p->name);
>   	kfree(p);
>   }
>   
>   /*
>    * This function unregisters and destroy all slave MTD objects which are
> - * attached to the given master MTD object.
> + * attached to the given master MTD object, recursively.
>    */
> -
> -int del_mtd_partitions(struct mtd_info *master)
> +static int do_del_mtd_partitions(struct mtd_info *master)
>   {
> -	struct mtd_part *slave, *next;
> +	struct mtd_info *slave, *next;
>   	int ret, err = 0;
>   
> -	debug("Deleting MTD partitions on \"%s\":\n", master->name);
> +	list_for_each_entry_safe(slave, next, &master->partitions, node) {
> +		if (mtd_has_partitions(slave))
> +			del_mtd_partitions(slave);
>   
> -	mutex_lock(&mtd_partitions_mutex);
> -	list_for_each_entry_safe(slave, next, &mtd_partitions, list)
> -		if (slave->master == master) {
> -			ret = del_mtd_device(&slave->mtd);
> -			if (ret < 0) {
> -				err = ret;
> -				continue;
> -			}
> -			list_del(&slave->list);
> -			free_partition(slave);
> +		debug("Deleting %s MTD partition\n", slave->name);
> +		ret = del_mtd_device(slave);
> +		if (ret < 0) {
> +			printk("Error when deleting partition \"%s\" (%d)\n",
> +			       slave->name, ret);
> +			err = ret;
> +			continue;
>   		}
> -	mutex_unlock(&mtd_partitions_mutex);
> +
> +		list_del(&slave->node);
> +		free_partition(slave);
> +	}
>   
>   	return err;
>   }
>   
> -static struct mtd_part *allocate_partition(struct mtd_info *master,
> -			const struct mtd_partition *part, int partno,
> -			uint64_t cur_offset)
> +int del_mtd_partitions(struct mtd_info *master)
>   {
> -	struct mtd_part *slave;
> +	int ret;
> +
> +	debug("Deleting MTD partitions on \"%s\":\n", master->name);
> +
> +	mutex_lock(&mtd_partitions_mutex);
> +	ret = do_del_mtd_partitions(master);
> +	mutex_unlock(&mtd_partitions_mutex);
> +
> +	return ret;
> +}
> +
> +static struct mtd_info *allocate_partition(struct mtd_info *master,
> +					   const struct mtd_partition *part,
> +					   int partno, uint64_t cur_offset)
> +{
> +	struct mtd_info *slave;
>   	char *name;
>   
>   	/* allocate the partition structure */
> @@ -648,85 +613,87 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>   	}
>   
>   	/* set up the MTD object for this partition */
> -	slave->mtd.type = master->type;
> -	slave->mtd.flags = master->flags & ~part->mask_flags;
> -	slave->mtd.size = part->size;
> -	slave->mtd.writesize = master->writesize;
> -	slave->mtd.writebufsize = master->writebufsize;
> -	slave->mtd.oobsize = master->oobsize;
> -	slave->mtd.oobavail = master->oobavail;
> -	slave->mtd.subpage_sft = master->subpage_sft;
> +	slave->type = master->type;
> +	slave->flags = master->flags & ~part->mask_flags;
> +	slave->size = part->size;
> +	slave->writesize = master->writesize;
> +	slave->writebufsize = master->writebufsize;
> +	slave->oobsize = master->oobsize;
> +	slave->oobavail = master->oobavail;
> +	slave->subpage_sft = master->subpage_sft;
>   
> -	slave->mtd.name = name;
> -	slave->mtd.owner = master->owner;
> +	slave->name = name;
> +	slave->owner = master->owner;
>   #ifndef __UBOOT__
> -	slave->mtd.backing_dev_info = master->backing_dev_info;
> +	slave->backing_dev_info = master->backing_dev_info;
>   
>   	/* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
>   	 * to have the same data be in two different partitions.
>   	 */
> -	slave->mtd.dev.parent = master->dev.parent;
> +	slave->dev.parent = master->dev.parent;
>   #endif
>   
>   	if (master->_read)
> -		slave->mtd._read = part_read;
> +		slave->_read = part_read;
>   	if (master->_write)
> -		slave->mtd._write = part_write;
> +		slave->_write = part_write;
>   
>   	if (master->_panic_write)
> -		slave->mtd._panic_write = part_panic_write;
> +		slave->_panic_write = part_panic_write;
>   
>   #ifndef __UBOOT__
>   	if (master->_point && master->_unpoint) {
> -		slave->mtd._point = part_point;
> -		slave->mtd._unpoint = part_unpoint;
> +		slave->_point = part_point;
> +		slave->_unpoint = part_unpoint;
>   	}
>   #endif
>   
>   	if (master->_get_unmapped_area)
> -		slave->mtd._get_unmapped_area = part_get_unmapped_area;
> +		slave->_get_unmapped_area = part_get_unmapped_area;
>   	if (master->_read_oob)
> -		slave->mtd._read_oob = part_read_oob;
> +		slave->_read_oob = part_read_oob;
>   	if (master->_write_oob)
> -		slave->mtd._write_oob = part_write_oob;
> +		slave->_write_oob = part_write_oob;
>   	if (master->_read_user_prot_reg)
> -		slave->mtd._read_user_prot_reg = part_read_user_prot_reg;
> +		slave->_read_user_prot_reg = part_read_user_prot_reg;
>   	if (master->_read_fact_prot_reg)
> -		slave->mtd._read_fact_prot_reg = part_read_fact_prot_reg;
> +		slave->_read_fact_prot_reg = part_read_fact_prot_reg;
>   	if (master->_write_user_prot_reg)
> -		slave->mtd._write_user_prot_reg = part_write_user_prot_reg;
> +		slave->_write_user_prot_reg = part_write_user_prot_reg;
>   	if (master->_lock_user_prot_reg)
> -		slave->mtd._lock_user_prot_reg = part_lock_user_prot_reg;
> +		slave->_lock_user_prot_reg = part_lock_user_prot_reg;
>   	if (master->_get_user_prot_info)
> -		slave->mtd._get_user_prot_info = part_get_user_prot_info;
> +		slave->_get_user_prot_info = part_get_user_prot_info;
>   	if (master->_get_fact_prot_info)
> -		slave->mtd._get_fact_prot_info = part_get_fact_prot_info;
> +		slave->_get_fact_prot_info = part_get_fact_prot_info;
>   	if (master->_sync)
> -		slave->mtd._sync = part_sync;
> +		slave->_sync = part_sync;
>   #ifndef __UBOOT__
>   	if (!partno && !master->dev.class && master->_suspend &&
>   	    master->_resume) {
> -			slave->mtd._suspend = part_suspend;
> -			slave->mtd._resume = part_resume;
> +			slave->_suspend = part_suspend;
> +			slave->_resume = part_resume;
>   	}
>   	if (master->_writev)
> -		slave->mtd._writev = part_writev;
> +		slave->_writev = part_writev;
>   #endif
>   	if (master->_lock)
> -		slave->mtd._lock = part_lock;
> +		slave->_lock = part_lock;
>   	if (master->_unlock)
> -		slave->mtd._unlock = part_unlock;
> +		slave->_unlock = part_unlock;
>   	if (master->_is_locked)
> -		slave->mtd._is_locked = part_is_locked;
> +		slave->_is_locked = part_is_locked;
>   	if (master->_block_isreserved)
> -		slave->mtd._block_isreserved = part_block_isreserved;
> +		slave->_block_isreserved = part_block_isreserved;
>   	if (master->_block_isbad)
> -		slave->mtd._block_isbad = part_block_isbad;
> +		slave->_block_isbad = part_block_isbad;
>   	if (master->_block_markbad)
> -		slave->mtd._block_markbad = part_block_markbad;
> -	slave->mtd._erase = part_erase;
> -	slave->master = master;
> +		slave->_block_markbad = part_block_markbad;
> +	slave->_erase = part_erase;
> +	slave->parent = master;
>   	slave->offset = part->offset;
> +	INIT_LIST_HEAD(&slave->partitions);
> +	INIT_LIST_HEAD(&slave->node);
>   
>   	if (slave->offset == MTDPART_OFS_APPEND)
>   		slave->offset = cur_offset;
> @@ -742,41 +709,41 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>   	}
>   	if (slave->offset == MTDPART_OFS_RETAIN) {
>   		slave->offset = cur_offset;
> -		if (master->size - slave->offset >= slave->mtd.size) {
> -			slave->mtd.size = master->size - slave->offset
> -							- slave->mtd.size;
> +		if (master->size - slave->offset >= slave->size) {
> +			slave->size = master->size - slave->offset
> +							- slave->size;
>   		} else {
>   			debug("mtd partition \"%s\" doesn't have enough space: %#llx < %#llx, disabled\n",
>   				part->name, master->size - slave->offset,
> -				slave->mtd.size);
> +				slave->size);
>   			/* register to preserve ordering */
>   			goto out_register;
>   		}
>   	}
> -	if (slave->mtd.size == MTDPART_SIZ_FULL)
> -		slave->mtd.size = master->size - slave->offset;
> +	if (slave->size == MTDPART_SIZ_FULL)
> +		slave->size = master->size - slave->offset;
>   
>   	debug("0x%012llx-0x%012llx : \"%s\"\n", (unsigned long long)slave->offset,
> -		(unsigned long long)(slave->offset + slave->mtd.size), slave->mtd.name);
> +		(unsigned long long)(slave->offset + slave->size), slave->name);
>   
>   	/* let's do some sanity checks */
>   	if (slave->offset >= master->size) {
>   		/* let's register it anyway to preserve ordering */
>   		slave->offset = 0;
> -		slave->mtd.size = 0;
> +		slave->size = 0;
>   		printk(KERN_ERR"mtd: partition \"%s\" is out of reach -- disabled\n",
>   			part->name);
>   		goto out_register;
>   	}
> -	if (slave->offset + slave->mtd.size > master->size) {
> -		slave->mtd.size = master->size - slave->offset;
> +	if (slave->offset + slave->size > master->size) {
> +		slave->size = master->size - slave->offset;
>   		printk(KERN_WARNING"mtd: partition \"%s\" extends beyond the end of device \"%s\" -- size truncated to %#llx\n",
> -			part->name, master->name, (unsigned long long)slave->mtd.size);
> +			part->name, master->name, (unsigned long long)slave->size);
>   	}
>   	if (master->numeraseregions > 1) {
>   		/* Deal with variable erase size stuff */
>   		int i, max = master->numeraseregions;
> -		u64 end = slave->offset + slave->mtd.size;
> +		u64 end = slave->offset + slave->size;
>   		struct mtd_erase_region_info *regions = master->eraseregions;
>   
>   		/* Find the first erase regions which is part of this
> @@ -789,44 +756,44 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>   
>   		/* Pick biggest erasesize */
>   		for (; i < max && regions[i].offset < end; i++) {
> -			if (slave->mtd.erasesize < regions[i].erasesize) {
> -				slave->mtd.erasesize = regions[i].erasesize;
> +			if (slave->erasesize < regions[i].erasesize) {
> +				slave->erasesize = regions[i].erasesize;
>   			}
>   		}
> -		BUG_ON(slave->mtd.erasesize == 0);
> +		BUG_ON(slave->erasesize == 0);
>   	} else {
>   		/* Single erase size */
> -		slave->mtd.erasesize = master->erasesize;
> +		slave->erasesize = master->erasesize;
>   	}
>   
> -	if ((slave->mtd.flags & MTD_WRITEABLE) &&
> -	    mtd_mod_by_eb(slave->offset, &slave->mtd)) {
> +	if ((slave->flags & MTD_WRITEABLE) &&
> +	    mtd_mod_by_eb(slave->offset, slave)) {
>   		/* Doesn't start on a boundary of major erase size */
>   		/* FIXME: Let it be writable if it is on a boundary of
>   		 * _minor_ erase size though */
> -		slave->mtd.flags &= ~MTD_WRITEABLE;
> +		slave->flags &= ~MTD_WRITEABLE;
>   		printk(KERN_WARNING"mtd: partition \"%s\" doesn't start on an erase block boundary -- force read-only\n",
>   			part->name);
>   	}
> -	if ((slave->mtd.flags & MTD_WRITEABLE) &&
> -	    mtd_mod_by_eb(slave->mtd.size, &slave->mtd)) {
> -		slave->mtd.flags &= ~MTD_WRITEABLE;
> +	if ((slave->flags & MTD_WRITEABLE) &&
> +	    mtd_mod_by_eb(slave->size, slave)) {
> +		slave->flags &= ~MTD_WRITEABLE;
>   		printk(KERN_WARNING"mtd: partition \"%s\" doesn't end on an erase block -- force read-only\n",
>   			part->name);
>   	}
>   
> -	slave->mtd.ecclayout = master->ecclayout;
> -	slave->mtd.ecc_step_size = master->ecc_step_size;
> -	slave->mtd.ecc_strength = master->ecc_strength;
> -	slave->mtd.bitflip_threshold = master->bitflip_threshold;
> +	slave->ecclayout = master->ecclayout;
> +	slave->ecc_step_size = master->ecc_step_size;
> +	slave->ecc_strength = master->ecc_strength;
> +	slave->bitflip_threshold = master->bitflip_threshold;
>   
>   	if (master->_block_isbad) {
>   		uint64_t offs = 0;
>   
> -		while (offs < slave->mtd.size) {
> +		while (offs < slave->size) {
>   			if (mtd_block_isbad(master, offs + slave->offset))
> -				slave->mtd.ecc_stats.badblocks++;
> -			offs += slave->mtd.erasesize;
> +				slave->ecc_stats.badblocks++;
> +			offs += slave->erasesize;
>   		}
>   	}
>   
> @@ -839,7 +806,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>   		      long long offset, long long length)
>   {
>   	struct mtd_partition part;
> -	struct mtd_part *p, *new;
> +	struct mtd_info *p, *new;
>   	uint64_t start, end;
>   	int ret = 0;
>   
> @@ -868,21 +835,20 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>   	end = offset + length;
>   
>   	mutex_lock(&mtd_partitions_mutex);
> -	list_for_each_entry(p, &mtd_partitions, list)
> -		if (p->master == master) {
> -			if ((start >= p->offset) &&
> -			    (start < (p->offset + p->mtd.size)))
> -				goto err_inv;
> +	list_for_each_entry(p, &master->partitions, node) {
> +		if ((start >= p->offset) &&
> +		    (start < (p->offset + p->size)))
> +			goto err_inv;
>   
> -			if ((end >= p->offset) &&
> -			    (end < (p->offset + p->mtd.size)))
> -				goto err_inv;
> -		}
> +		if ((end >= p->offset) &&
> +		    (end < (p->offset + p->size)))
> +			goto err_inv;
> +	}
>   
> -	list_add(&new->list, &mtd_partitions);
> +	list_add_tail(&new->node, &master->partitions);
>   	mutex_unlock(&mtd_partitions_mutex);
>   
> -	add_mtd_device(&new->mtd);
> +	add_mtd_device(new);
>   
>   	return ret;
>   err_inv:
> @@ -894,18 +860,17 @@ EXPORT_SYMBOL_GPL(mtd_add_partition);
>   
>   int mtd_del_partition(struct mtd_info *master, int partno)
>   {
> -	struct mtd_part *slave, *next;
> +	struct mtd_info *slave, *next;
>   	int ret = -EINVAL;
>   
>   	mutex_lock(&mtd_partitions_mutex);
> -	list_for_each_entry_safe(slave, next, &mtd_partitions, list)
> -		if ((slave->master == master) &&
> -		    (slave->mtd.index == partno)) {
> -			ret = del_mtd_device(&slave->mtd);
> +	list_for_each_entry_safe(slave, next, &master->partitions, node)
> +		if (slave->index == partno) {
> +			ret = del_mtd_device(slave);
>   			if (ret < 0)
>   				break;
>   
> -			list_del(&slave->list);
> +			list_del(&slave->node);
>   			free_partition(slave);
>   			break;
>   		}
> @@ -929,20 +894,10 @@ int add_mtd_partitions(struct mtd_info *master,
>   		       const struct mtd_partition *parts,
>   		       int nbparts)
>   {
> -	struct mtd_part *slave;
> +	struct mtd_info *slave;
>   	uint64_t cur_offset = 0;
>   	int i;
>   
> -#ifdef __UBOOT__
> -	/*
> -	 * Need to init the list here, since LIST_INIT() does not
> -	 * work on platforms where relocation has problems (like MIPS
> -	 * & PPC).
> -	 */
> -	if (mtd_partitions.next == NULL)
> -		INIT_LIST_HEAD(&mtd_partitions);
> -#endif
> -
>   	debug("Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
>   
>   	for (i = 0; i < nbparts; i++) {
> @@ -951,12 +906,12 @@ int add_mtd_partitions(struct mtd_info *master,
>   			return PTR_ERR(slave);
>   
>   		mutex_lock(&mtd_partitions_mutex);
> -		list_add(&slave->list, &mtd_partitions);
> +		list_add_tail(&slave->node, &master->partitions);
>   		mutex_unlock(&mtd_partitions_mutex);
>   
> -		add_mtd_device(&slave->mtd);
> +		add_mtd_device(slave);
>   
> -		cur_offset = slave->offset + slave->mtd.size;
> +		cur_offset = slave->offset + slave->size;
>   	}
>   
>   	return 0;
> @@ -1059,29 +1014,12 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>   }
>   #endif
>   
> -int mtd_is_partition(const struct mtd_info *mtd)
> -{
> -	struct mtd_part *part;
> -	int ispart = 0;
> -
> -	mutex_lock(&mtd_partitions_mutex);
> -	list_for_each_entry(part, &mtd_partitions, list)
> -		if (&part->mtd == mtd) {
> -			ispart = 1;
> -			break;
> -		}
> -	mutex_unlock(&mtd_partitions_mutex);
> -
> -	return ispart;
> -}
> -EXPORT_SYMBOL_GPL(mtd_is_partition);
> -
>   /* Returns the size of the entire flash chip */
>   uint64_t mtd_get_device_size(const struct mtd_info *mtd)
>   {
> -	if (!mtd_is_partition(mtd))
> -		return mtd->size;
> +	if (mtd_is_partition(mtd))
> +		return mtd->parent->size;
>   
> -	return PART(mtd)->master->size;
> +	return mtd->size;
>   }
>   EXPORT_SYMBOL_GPL(mtd_get_device_size);
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index b8c2c3fd59..ba582d38bc 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -307,6 +307,27 @@ struct mtd_info {
>   	struct udevice *dev;
>   #endif
>   	int usecount;
> +
> +	/* MTD devices do not have any parent. MTD partitions do. */
> +	struct mtd_info *parent;
> +
> +	/*
> +	 * Offset of the partition relatively to the parent offset.
> +	 * Is 0 for real MTD devices (ie. not partitions).
> +	 */
> +	u64 offset;
> +
> +	/*
> +	 * List node used to add an MTD partition to the parent
> +	 * partition list.
> +	 */
> +	struct list_head node;
> +
> +	/*
> +	 * List of partitions attached to this MTD device (the parent
> +	 * MTD device can itself be a partition).
> +	 */
> +	struct list_head partitions;
>   };
>   
>   #if IS_ENABLED(CONFIG_DM)
> @@ -334,6 +355,16 @@ static inline const struct device_node *mtd_get_of_node(struct mtd_info *mtd)
>   }
>   #endif
>   
> +static inline bool mtd_is_partition(const struct mtd_info *mtd)
> +{
> +	return mtd->parent;
> +}
> +
> +static inline bool mtd_has_partitions(const struct mtd_info *mtd)
> +{
> +	return !list_empty(&mtd->partitions);
> +}
> +
>   int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,
>   		      struct mtd_oob_region *oobecc);
>   int mtd_ooblayout_find_eccregion(struct mtd_info *mtd, int eccbyte,
> diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
> index 082a4966ea..b8db1b0a58 100644
> --- a/include/linux/mtd/partitions.h
> +++ b/include/linux/mtd/partitions.h
> @@ -81,7 +81,6 @@ extern void register_mtd_parser(struct mtd_part_parser *parser);
>   extern void deregister_mtd_parser(struct mtd_part_parser *parser);
>   #endif
>   
> -int mtd_is_partition(const struct mtd_info *mtd);
>   int mtd_add_partition(struct mtd_info *master, const char *name,
>   		      long long offset, long long length);
>   int mtd_del_partition(struct mtd_info *master, int partno);
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 10/13] cmd: mtd: add 'mtd' command

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> There should not be a 'nand' command, a 'sf' command and certainly not
> a new 'spi-nand' command. Write a 'mtd' command instead to manage all
> MTD devices/partitions at once. This should be the preferred way to
> access any MTD device.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Jagan Teki <jagan@openedev.com>
> ---
>   cmd/Kconfig          |  10 +-
>   cmd/Makefile         |   1 +
>   cmd/mtd.c            | 517 +++++++++++++++++++++++++++++++++++++++++++
>   drivers/mtd/Makefile |   2 +-
>   include/mtd.h        |   1 +
>   5 files changed, 528 insertions(+), 3 deletions(-)
>   create mode 100644 cmd/mtd.c
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index ef43ed8dda..0778d2ecff 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -847,6 +847,12 @@ config CMD_MMC_SWRITE
>   	  Enable support for the "mmc swrite" command to write Android sparse
>   	  images to eMMC.
>   
> +config CMD_MTD
> +	bool "mtd"
> +	select MTD_PARTITIONS
> +	help
> +	  MTD commands support.
> +
>   config CMD_NAND
>   	bool "nand"
>   	default y if NAND_SUNXI
> @@ -1671,14 +1677,14 @@ config CMD_MTDPARTS
>   
>   config MTDIDS_DEFAULT
>   	string "Default MTD IDs"
> -	depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> +	depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>   	help
>   	  Defines a default MTD IDs list for use with MTD partitions in the
>   	  Linux MTD command line partitions format.
>   
>   config MTDPARTS_DEFAULT
>   	string "Default MTD partition scheme"
> -	depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> +	depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>   	help
>   	  Defines a default MTD partitioning scheme in the Linux MTD command
>   	  line partitions format
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 323f1fd2c7..32fd102189 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -90,6 +90,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..32295fe86c
> --- /dev/null
> +++ b/cmd/mtd.c
> @@ -0,0 +1,517 @@
> +// 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 <command.h>
> +#include <common.h>
> +#include <console.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <malloc.h>
> +#include <mapmem.h>
> +#include <mtd.h>
> +#include <dm/device.h>
> +#include <dm/uclass-internal.h>
> +
> +#define MTD_NAME_MAX_LEN 20
> +
> +static char *old_mtdparts;
> +
> +static void mtd_dump_buf(u8 *buf, uint len, uint offset)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < len; ) {
> +		printf("0x%08x:\t", offset + 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_parts(struct mtd_info *mtd, int level)
> +{
> +	struct mtd_info *part;
> +	int i;
> +
> +	if (list_empty(&mtd->partitions))
> +		return;
> +
> +	list_for_each_entry(part, &mtd->partitions, node) {
> +		for (i = 0; i < level; i++)
> +			printf("\t");
> +		printf("* %s\n", part->name);
> +		for (i = 0; i < level; i++)
> +			printf("\t");
> +		printf("  > Offset: 0x%llx bytes\n", part->offset);
> +		for (i = 0; i < level; i++)
> +			printf("\t");
> +		printf("  > Size: 0x%llx bytes\n", part->size);
> +
> +		mtd_show_parts(part, level + 1);
> +	}
> +}
> +
> +static void mtd_show_device(struct mtd_info *mtd)
> +{
> +	/* Device */
> +	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");
> +
> +	/* MTD device information */
> +	printf("  > type: ");
> +	switch (mtd->type) {
> +	case MTD_RAM:
> +		printf("RAM\n");
> +		break;
> +	case MTD_ROM:
> +		printf("ROM\n");
> +		break;
> +	case MTD_NORFLASH:
> +		printf("NOR flash\n");
> +		break;
> +	case MTD_NANDFLASH:
> +		printf("NAND flash\n");
> +		break;
> +	case MTD_DATAFLASH:
> +		printf("Data flash\n");
> +		break;
> +	case MTD_UBIVOLUME:
> +		printf("UBI volume\n");
> +		break;
> +	case MTD_MLCNANDFLASH:
> +		printf("MLC NAND flash\n");
> +		break;
> +	case MTD_ABSENT:
> +	default:
> +		printf("Unknown\n");
> +		break;
> +	}
> +
> +	printf("  > Size: 0x%llx bytes\n", mtd->size);
> +	printf("  > Block: 0x%x bytes\n", mtd->erasesize);
> +	printf("  > Min I/O: 0x%x bytes\n", mtd->writesize);
> +
> +	if (mtd->oobsize) {
> +		printf("  > OOB size: %u bytes\n", mtd->oobsize);
> +		printf("  > OOB available: %u bytes\n", mtd->oobavail);
> +	}
> +
> +	if (mtd->ecc_strength) {
> +		printf("  > ECC strength: %u bits\n", mtd->ecc_strength);
> +		printf("  > ECC step size: %u bytes\n", mtd->ecc_step_size);
> +		printf("  > Bitflip threshold: %u bits\n",
> +		       mtd->bitflip_threshold);
> +	}
> +
> +	/* MTD partitions, if any */
> +	mtd_show_parts(mtd, 1);
> +}
> +
> +#if IS_ENABLED(CONFIG_MTD)
> +static void mtd_probe_uclass_mtd_devs(void)
> +{
> +	struct udevice *dev;
> +	int idx = 0;
> +
> +	/* Probe devices with DM compliant drivers */
> +	while (!uclass_find_device(UCLASS_MTD, idx, &dev) && dev) {
> +		mtd_probe(dev);
> +		idx++;
> +	}
> +}
> +#else
> +static void mtd_probe_uclass_mtd_devs(void) { }
> +#endif

Does it make sense to support this new command without CONFIG_MTD
enabled? Do you have a use case for this?

> +
> +int mtd_probe_devices(void)
> +{
> +	const char *mtdparts = env_get("mtdparts");
> +	bool remaining_partitions = true;
> +	struct mtd_info *mtd;
> +	int i;
> +
> +	mtd_probe_uclass_mtd_devs();
> +
> +	/* Check if mtdparts changed since last call, if not, just exit */
> +	if (!strcmp(mtdparts, old_mtdparts))
> +		return CMD_RET_SUCCESS;
> +
> +	/* Update the local copy of mtdparts */
> +	free(old_mtdparts);
> +	old_mtdparts = strdup(mtdparts);
> +
> +	/* If at least one partition is still in use, do not delete anything */
> +	mtd_for_each_device(mtd) {
> +		if (mtd->usecount) {
> +			printf("Partition \"%s\" already in use, aborting\n",
> +			       mtd->name);
> +			return CMD_RET_FAILURE;
> +		}
> +	}
> +
> +	/*
> +	 * Everything looks clear, remove all partitions. It is not safe to
> +	 * remove entries from the mtd_for_each_device loop as it uses idr
> +	 * indexes and the partitions removal is done in bulk (all partitions of
> +	 * one device at the same time), so break and iterate from start each
> +	 * time a new partition is found and deleted.
> +	 */
> +	while (remaining_partitions) {
> +		remaining_partitions = false;
> +		mtd_for_each_device(mtd) {
> +			if (!mtd_is_partition(mtd) && mtd_has_partitions(mtd)) {
> +				del_mtd_partitions(mtd);
> +				remaining_partitions = true;
> +				break;
> +			}
> +		}
> +	}
> +
> +	/* Start the parsing by ignoring the extra 'mtdparts=' prefix, if any */
> +	if (strstr(mtdparts, "mtdparts="))
> +		mtdparts += 9;
> +
> +	/* For each MTD device in mtdparts */
> +	while (mtdparts[0] != '\0') {
> +		char mtdname[MTD_NAME_MAX_LEN], *colon;
> +		struct mtd_partition *parts;
> +		int mtdname_len, len;
> +		int ret;
> +
> +		colon = strchr(mtdparts, ':');
> +		if (!colon) {
> +			printf("Wrong mtdparts: %s\n", mtdparts);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		mtdname_len = colon - mtdparts;
> +		strncpy(mtdname, mtdparts, mtdname_len);
> +		mtdname[mtdname_len] = '\0';
> +		/* Move the pointer forward (including the ':') */
> +		mtdparts += mtdname_len + 1;
> +		mtd = get_mtd_device_nm(mtdname);
> +		if (IS_ERR_OR_NULL(mtd)) {
> +			char altname[MTD_NAME_MAX_LEN];
> +
> +			/*
> +			 * The MTD device named "mtdname" does not exist. Try to
> +			 * find a correspondance with an MTD device having the
> +			 * same type and number as defined in the mtdids.
> +			 */
> +			printf("No device named %s\n", mtdname);
> +			ret = mtd_search_alternate_name(mtdname, altname,
> +							MTD_NAME_MAX_LEN);
> +			if (ret)  {
> +				printf("Did not found an equivalent in mtdids\n");
> +				return CMD_RET_FAILURE;
> +			}
> +
> +			mtd = get_mtd_device_nm(altname);
> +			if (IS_ERR_OR_NULL(mtd)) {
> +				printf("No device named %s\n", altname);
> +				return CMD_RET_FAILURE;
> +			}
> +		}
> +		put_mtd_device(mtd);
> +
> +		/*
> +		 * Parse the MTD device partitions. It will update the mtdparts
> +		 * pointer, create an array of parts (that must be freed), and
> +		 * return the number of partition structures in the array.
> +		 */
> +		ret = mtd_parse_partitions(mtd, &mtdparts, &parts, &len);
> +		if (ret) {
> +			printf("Could not parse device %s\n", mtd->name);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		if (!len)
> +			continue;
> +
> +		/*
> +		 * Create the new MTD partitions and free the structures
> +		 * allocated during the parsing.
> +		 */
> +		add_mtd_partitions(mtd, parts, len);
> +
> +		for (i = 0; i < len; i++)
> +			free(&parts[i].name);
> +		free(parts);
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
> +{
> +	do_div(len, mtd->writesize);
> +
> +	return len;
> +}
> +
> +static bool mtd_is_aligned_with_min_io_size(struct mtd_info *mtd, u64 size)
> +{
> +	return do_div(size, mtd->writesize);
> +}
> +
> +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> +{
> +	return do_div(size, mtd->erasesize);
> +}
> +
> +static int do_mtd_list(void)
> +{
> +	struct mtd_info *mtd;
> +	int dev_nb = 0;
> +
> +#if IS_ENABLED(CONFIG_MTD)
> +	/* Ensure all devices (and their partitions) are probed */
> +	mtd_probe_devices();
> +#endif
> +
> +	printf("List of MTD devices:\n");
> +	mtd_for_each_device(mtd) {
> +		if (!mtd_is_partition(mtd))
> +			mtd_show_device(mtd);
> +
> +		dev_nb++;
> +	}
> +
> +	if (!dev_nb) {
> +		printf("No MTD device found\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	struct mtd_info *mtd;
> +	const char *cmd;
> +	char *mtdname;
> +	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;
> +
> +	mtdname = argv[2];
> +	mtd_probe_devices();
> +	mtd = get_mtd_device_nm(mtdname);
> +	if (IS_ERR_OR_NULL(mtd)) {
> +		printf("MTD device %s not found, ret %ld\n",
> +		       mtdname, PTR_ERR(mtd));
> +		return CMD_RET_FAILURE;
> +	}
> +	put_mtd_device(mtd);
> +
> +	argc -= 3;
> +	argv += 3;
> +
> +	/* Do the parsing */
> +	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "dump", 4) ||
> +	    !strncmp(cmd, "write", 5)) {
> +		struct mtd_oob_ops io_op = {};
> +		bool dump, read, raw, woob;
> +		uint user_addr = 0;
> +		uint nb_pages;
> +		u8 *buf;
> +		u64 off, len, default_len;
> +		u32 oob_len;
> +
> +		dump = !strncmp(cmd, "dump", 4);
> +		read = dump || !strncmp(cmd, "read", 4);
> +		raw = strstr(cmd, ".raw");
> +		woob = strstr(cmd, ".oob");
> +
> +		if (!dump) {
> +			if (!argc)
> +				return CMD_RET_USAGE;
> +
> +			user_addr = simple_strtoul(argv[0], NULL, 16);
> +			argc--;
> +			argv++;
> +		}
> +
> +		default_len = dump ? mtd->writesize : mtd->size;
> +		off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
> +		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) :
> +				 default_len;
> +		nb_pages = mtd_len_to_pages(mtd, len);
> +		oob_len = woob ? nb_pages * mtd->oobsize : 0;
> +
> +		if (mtd_is_aligned_with_min_io_size(mtd, off)) {
> +			printf("Offset not aligned with a page (0x%x)\n",
> +			       mtd->writesize);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		if (mtd_is_aligned_with_min_io_size(mtd, len)) {
> +			printf("Size not a multiple of a page (0x%x)\n",
> +			       mtd->writesize);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		if (dump)
> +			buf = kmalloc(len + oob_len, GFP_KERNEL);
> +		else
> +			buf = map_sysmem(user_addr, 0);
> +
> +		if (!buf) {
> +			printf("Could not map/allocate the user buffer\n");
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		printf("%s %lldB (%d page(s)) at offset 0x%08llx%s%s\n",
> +		       read ? "Reading" : "Writing", len, nb_pages, off,
> +		       raw ? " [raw]" : "", woob ? " [oob]" : "");
> +
> +		io_op.mode = raw ? MTD_OPS_RAW : MTD_OPS_AUTO_OOB;
> +		io_op.len = len;
> +		io_op.ooblen = oob_len;
> +		io_op.datbuf = buf;
> +		io_op.oobbuf = woob ? &buf[len] : NULL;
> +
> +		if (read)
> +			ret = mtd_read_oob(mtd, off, &io_op);
> +		else
> +			ret = mtd_write_oob(mtd, off, &io_op);
> +
> +		if (!ret && dump) {
> +			uint page;
> +
> +			for (page = 0; page < nb_pages; page++) {
> +				u64 data_off = page * mtd->writesize;
> +
> +				printf("\nDump %d data bytes from 0x%08llx:\n",
> +				       mtd->writesize, data_off);
> +				mtd_dump_buf(buf, mtd->writesize, data_off);
> +
> +				if (woob) {
> +					u64 oob_off = page * mtd->oobsize;
> +
> +					printf("Dump %d OOB bytes from page at 0x%08llx:\n",
> +					       mtd->oobsize, data_off);
> +					mtd_dump_buf(&buf[len + oob_off],
> +						     mtd->oobsize, 0);
> +				}
> +			}
> +		}
> +
> +		if (dump)
> +			kfree(buf);
> +		else
> +			unmap_sysmem(buf);
> +
> +		if (ret) {
> +			printf("%s on %s failed with error %d\n",
> +			       read ? "Read" : "Write", mtd->name, ret);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +	} else if (!strcmp(cmd, "erase")) {
> +		bool scrub = strstr(cmd, ".dontskipbad");
> +		struct erase_info erase_op = {};
> +		u64 off, len;
> +
> +		off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
> +		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->size;
> +
> +		if (mtd_is_aligned_with_block_size(mtd, off)) {
> +			printf("Offset not aligned with a block (0x%x)\n",
> +			       mtd->erasesize);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		if (mtd_is_aligned_with_block_size(mtd, len)) {
> +			printf("Size not a multiple of a block (0x%x)\n",
> +			       mtd->erasesize);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		printf("Erasing 0x%08llx ... 0x%08llx (%d eraseblock(s))\n",
> +		       off, off + len - 1, mtd_div_by_eb(len, mtd));
> +
> +		erase_op.mtd = mtd;
> +		erase_op.addr = off;
> +		erase_op.len = len;
> +		erase_op.scrub = scrub;
> +
> +		while (erase_op.len) {
> +			ret = mtd_erase(mtd, &erase_op);
> +
> +			/* Abort if its not an bad block error */
> +			if (ret != -EIO)
> +				break;
> +
> +			printf("Skipping bad block at 0x%08llx\n",
> +			       erase_op.fail_addr);
> +
> +			/* Skip bad block and continue behind it */
> +			erase_op.len -= erase_op.fail_addr - erase_op.addr;
> +			erase_op.len -= mtd->erasesize;
> +			erase_op.addr = erase_op.fail_addr + mtd->erasesize;
> +		}
> +	} else {
> +		return CMD_RET_USAGE;
> +	}
> +
> +	if (ret)
> +		return CMD_RET_FAILURE;
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +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> <addr> [<off> [<size>]]\n"
> +	"mtd dump[.raw][.oob]    <name>        [<off> [<size>]]\n"
> +	"mtd write[.raw][.oob]   <name> <addr> [<off> [<size>]]\n"
> +	"mtd erase[.dontskipbad] <name>        [<off> [<size>]]\n"
> +	"\n"
> +	"With:\n"
> +	"\t<name>: NAND partition/chip name\n"
> +	"\t<addr>: user address from/to which data will be retrieved/stored\n"
> +	"\t<off>: offset in <name> in bytes (default: start of the part)\n"
> +	"\t\t* must be block-aligned for erase\n"
> +	"\t\t* must be page-aligned otherwise\n"
> +	"\t<size>: length of the operation in bytes (default: the entire device)\n"
> +	"\t\t* must be a multiple of a block for erase\n"
> +	"\t\t* must be a multiple of a page otherwise (special case: default is a page with dump)\n"
> +#endif
> +	"";
> +
> +U_BOOT_CMD(mtd, 10, 1, do_mtd, "MTD utils", mtd_help_text);
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index cdf515256a..22ceda93c0 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -3,7 +3,7 @@
>   # (C) Copyright 2000-2007
>   # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>   
> -ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND)$(CONFIG_CMD_SF)))
> +ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND)$(CONFIG_CMD_SF)$(CONFIG_CMD_MTD)))
>   obj-y += mtdcore.o mtd_uboot.o
>   endif
>   obj-$(CONFIG_MTD) += mtd-uclass.o
> diff --git a/include/mtd.h b/include/mtd.h
> index 6e6da3002f..04b1c50ac7 100644
> --- a/include/mtd.h
> +++ b/include/mtd.h
> @@ -20,5 +20,6 @@ static inline struct mtd_info *mtd_get_info(struct udevice *dev)
>   }
>   
>   int mtd_probe(struct udevice *dev);
> +int mtd_probe_devices(void);
>   
>   #endif	/* _MTD_H_ */
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 11/13] cmd: mtdparts: try to probe the MTD devices as a fallback

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> Current implementation of mtdparts command errors out if the desired MTD
> device is not found. Fallback to the new probe function in this case
> before erroring out.
> 
> This will the save the user the need to call something like 'mtd list'
> before mtdparts.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Jagan Teki <jagan@openedev.com>
> ---
>   cmd/mtdparts.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
> index f7ed1a0779..a1102c3fc5 100644
> --- a/cmd/mtdparts.c
> +++ b/cmd/mtdparts.c
> @@ -79,6 +79,10 @@
>   #include <linux/err.h>
>   #include <linux/mtd/mtd.h>
>   
> +#if defined(CONFIG_MTD)
> +#include <mtd.h>
> +#endif
> +
>   #if defined(CONFIG_CMD_NAND)
>   #include <linux/mtd/rawnand.h>
>   #include <nand.h>
> @@ -307,9 +311,15 @@ static int get_mtd_info(u8 type, u8 num, struct mtd_info **mtd)
>   
>   	sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(type), num);
>   	*mtd = get_mtd_device_nm(mtd_dev);
> -	if (IS_ERR(*mtd)) {
> -		printf("Device %s not found!\n", mtd_dev);
> -		return 1;
> +	if (IS_ERR_OR_NULL(*mtd)) {
> +#ifdef CONFIG_MTD
> +		mtd_probe_devices();
> +		*mtd = get_mtd_device_nm(mtd_dev);
> +#endif
> +		if (IS_ERR_OR_NULL(*mtd)) {
> +			printf("Device %s not found!\n", mtd_dev);
> +			return 1;
> +		}
>   	}
>   	put_mtd_device(*mtd);
>   
> 

This is most likely the use case for CMD_MTD without MTD, correct?

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 12/13] cmd: ubi: clean the partition handling

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> UBI should not mess with MTD partitions, now that the partitions are
> handled in a clean way, clean the ubi command and avoid using this
> uneeded extra-glue to reference the devices.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   cmd/Kconfig |   2 ++
>   cmd/ubi.c   | 100 +++++++++++++++-------------------------------------
>   2 files changed, 31 insertions(+), 71 deletions(-)

Nice diffstat. ;)

> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 0778d2ecff..4deec0b238 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1815,6 +1815,8 @@ config CMD_UBI
>   	  capabilities. Please, consult the MTD web site for more details
>   	  (www.linux-mtd.infradead.org). Activate this option if you want
>   	  to use U-Boot UBI commands.
> +	  It is also strongly encouraged to also enable CONFIG_MTD to get full
> +	  partition support.
>   
>   config CMD_UBIFS
>   	tristate "Enable UBIFS - Unsorted block images filesystem commands"
> diff --git a/cmd/ubi.c b/cmd/ubi.c
> index 0a3405a3b1..af0cea2ca5 100644
> --- a/cmd/ubi.c
> +++ b/cmd/ubi.c
> @@ -15,6 +15,9 @@
>   #include <command.h>
>   #include <exports.h>
>   #include <memalign.h>
> +#if defined(CONFIG_MTD)
> +#include <mtd.h>
> +#endif
>   #include <nand.h>
>   #include <onenand_uboot.h>
>   #include <linux/mtd/mtd.h>
> @@ -29,17 +32,6 @@
>   
>   /* Private own data */
>   static struct ubi_device *ubi;
> -static char buffer[80];
> -static int ubi_initialized;
> -
> -struct selected_dev {
> -	char part_name[80];
> -	int selected;
> -	int nr;
> -	struct mtd_info *mtd_info;
> -};
> -
> -static struct selected_dev ubi_dev;
>   
>   #ifdef CONFIG_CMD_UBIFS
>   int ubifs_is_mounted(void);
> @@ -404,43 +396,24 @@ int ubi_volume_read(char *volume, char *buf, size_t size)
>   	return err;
>   }
>   
> -static int ubi_dev_scan(struct mtd_info *info, char *ubidev,
> -		const char *vid_header_offset)
> +static int ubi_dev_scan(struct mtd_info *info, const char *vid_header_offset)
>   {
> -	struct mtd_device *dev;
> -	struct part_info *part;
> -	struct mtd_partition mtd_part;
>   	char ubi_mtd_param_buffer[80];
> -	u8 pnum;
>   	int err;
>   
> -	if (find_dev_and_part(ubidev, &dev, &pnum, &part) != 0)
> -		return 1;
> +	if (!vid_header_offset)
> +		sprintf(ubi_mtd_param_buffer, "%s", info->name);
> +	else
> +		sprintf(ubi_mtd_param_buffer, "%s,%s", info->name,
> +			vid_header_offset);
>   
> -	sprintf(buffer, "mtd=%d", pnum);
> -	memset(&mtd_part, 0, sizeof(mtd_part));
> -	mtd_part.name = buffer;
> -	mtd_part.size = part->size;
> -	mtd_part.offset = part->offset;
> -	add_mtd_partitions(info, &mtd_part, 1);
> -
> -	strcpy(ubi_mtd_param_buffer, buffer);
> -	if (vid_header_offset)
> -		sprintf(ubi_mtd_param_buffer, "mtd=%d,%s", pnum,
> -				vid_header_offset);
>   	err = ubi_mtd_param_parse(ubi_mtd_param_buffer, NULL);
> -	if (err) {
> -		del_mtd_partitions(info);
> +	if (err)
>   		return -err;
> -	}
>   
>   	err = ubi_init();
> -	if (err) {
> -		del_mtd_partitions(info);
> +	if (err)
>   		return -err;
> -	}
> -
> -	ubi_initialized = 1;
>   
>   	return 0;
>   }
> @@ -465,50 +438,35 @@ int ubi_detach(void)
>   	/*
>   	 * Call ubi_exit() before re-initializing the UBI subsystem
>   	 */
> -	if (ubi_initialized) {
> +	if (ubi)
>   		ubi_exit();
> -		del_mtd_partitions(ubi_dev.mtd_info);
> -		ubi_initialized = 0;
> -	}
>   
> -	ubi_dev.selected = 0;
> +	ubi = NULL;
> +
>   	return 0;
>   }
>   
>   int ubi_part(char *part_name, const char *vid_header_offset)
>   {
> +	struct mtd_info *mtd;
>   	int err = 0;
> -	char mtd_dev[16];
> -	struct mtd_device *dev;
> -	struct part_info *part;
> -	u8 pnum;
>   
>   	ubi_detach();
> -	/*
> -	 * Search the mtd device number where this partition
> -	 * is located
> -	 */
> -	if (find_dev_and_part(part_name, &dev, &pnum, &part)) {
> +
> +#ifdef CONFIG_MTD
> +	mtd_probe_devices();
> +#endif
> +	mtd = get_mtd_device_nm(part_name);
> +	if (IS_ERR(mtd)) {
>   		printf("Partition %s not found!\n", part_name);
>   		return 1;
>   	}
> -	sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(dev->id->type), dev->id->num);
> -	ubi_dev.mtd_info = get_mtd_device_nm(mtd_dev);
> -	if (IS_ERR(ubi_dev.mtd_info)) {
> -		printf("Partition %s not found on device %s!\n", part_name,
> -		       mtd_dev);
> -		return 1;
> -	}
> +	put_mtd_device(mtd);
>   
> -	ubi_dev.selected = 1;
> -
> -	strcpy(ubi_dev.part_name, part_name);
> -	err = ubi_dev_scan(ubi_dev.mtd_info, ubi_dev.part_name,
> -			vid_header_offset);
> +	err = ubi_dev_scan(mtd, vid_header_offset);
>   	if (err) {
>   		printf("UBI init error %d\n", err);
>   		printf("Please check, if the correct MTD partition is used (size big enough?)\n");
> -		ubi_dev.selected = 0;
>   		return err;
>   	}
>   
> @@ -539,13 +497,13 @@ static int do_ubi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   
>   		/* Print current partition */
>   		if (argc == 2) {
> -			if (!ubi_dev.selected) {
> -				printf("Error, no UBI device/partition selected!\n");
> +			if (!ubi) {
> +				printf("Error, no UBI device selected!\n");
>   				return 1;
>   			}
>   
> -			printf("Device %d: %s, partition %s\n",
> -			       ubi_dev.nr, ubi_dev.mtd_info->name, ubi_dev.part_name);
> +			printf("Device %d: %s, MTD partition %s\n",
> +			       ubi->ubi_num, ubi->ubi_name, ubi->mtd->name);
>   			return 0;
>   		}
>   
> @@ -558,8 +516,8 @@ static int do_ubi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   		return ubi_part(argv[2], vid_header_offset);
>   	}
>   
> -	if ((strcmp(argv[1], "part") != 0) && (!ubi_dev.selected)) {
> -		printf("Error, no UBI device/partition selected!\n");
> +	if ((strcmp(argv[1], "part") != 0) && !ubi) {
> +		printf("Error, no UBI device selected!\n");
>   		return 1;
>   	}
>   
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 13/13] cmd: mtdparts: describe as legacy

[Stefan Roese]

On 31.08.2018 16:57, Miquel Raynal wrote:
> The 'mtdparts' command is not needed anymore. While the environment
> variable is still valid (and useful), the command has been replaced by
> 'mtd' which is much more close to the MTD stack and do not add its own
> specific glue. The 'mtdids' variable, only used by the 'mtdparts'
> command is also useless if the right MTD device name is used in the
> 'mtdparts' variable.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   cmd/Kconfig | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 4deec0b238..0786663f4a 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1673,7 +1673,11 @@ config CMD_MTDPARTS
>   	bool "MTD partition support"
>   	select MTD_DEVICE if (CMD_NAND || NAND)
>   	help
> -	  MTD partition support
> +	  MTD partitioning tool support.
> +	  It is strongly encouraged to avoid using this command
> +	  anymore. One can still declare the partitions in the
> +	  mtdparts environment variable but better use the MTD stack
> +	  and the mtd command instead than this one.
>   
>   config MTDIDS_DEFAULT
>   	string "Default MTD IDs"
> @@ -1681,6 +1685,10 @@ config MTDIDS_DEFAULT
>   	help
>   	  Defines a default MTD IDs list for use with MTD partitions in the
>   	  Linux MTD command line partitions format.
> +	  Declaration of this environment variable is not useful
> +	  anymore when using the right MTD names in mtdparts along
> +	  with the use of the 'mtd' command instead of the legacy
> +	  'mtdparts'.
>   
>   config MTDPARTS_DEFAULT
>   	string "Default MTD partition scheme"
> 

Very nice work overall. :)

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v7 01/13] lib: strto: parse all lowercase metric prefixes in ustrtoul[l]

[Miquel Raynal]

Hi Stefan,

Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 10:43:01 +0200:

> On 31.08.2018 16:57, Miquel Raynal wrote:
> > Both ustrtoul and ustrtoull interpret 1k but not 1m or 1g. Even if the
> > SI symbols for Mega and Giga are 'M' and 'G', certain entries of
> > eg. mtdparts also use (wrongly) the metric prefix 'm' and 'g'.  
> > > I do not see how parsing lowercase prefixes could break anything, so  
> > parse them like their uppercase counterpart.  
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> > ---
> >   lib/strto.c | 4 ++++
> >   1 file changed, 4 insertions(+)  
> > > diff --git a/lib/strto.c b/lib/strto.c  
> > index 7f6076909a..84f8d92d57 100644
> > --- a/lib/strto.c
> > +++ b/lib/strto.c
> > @@ -87,9 +87,11 @@ unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
> >   	unsigned long result = simple_strtoul(cp, endp, base);
> >   	switch (**endp) {
> >   	case 'G':
> > +	case 'g':
> >   		result *= 1024;
> >   		/* fall through */
> >   	case 'M':
> > +	case 'm':
> >   		result *= 1024;
> >   		/* fall through */
> >   	case 'K':
> > @@ -110,9 +112,11 @@ unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
> >   	unsigned long long result = simple_strtoull(cp, endp, base);
> >   	switch (**endp) {
> >   	case 'G':
> > +	case 'g':
> >   		result *= 1024;
> >   		/* fall through */
> >   	case 'M':
> > +	case 'm':  
> 
> Wouldn't it be better, to use tolower() on the char and drop all the
> upper case checks completely - also for the 'K' case?

Sure, I can do that!

Thanks,
Miquèl

[U-Boot] [PATCH v7 02/13] lib: strto: fix metric suffix parsing in strtoul[l]

[Miquel Raynal]

Hi Stefan,

Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 10:48:25 +0200:

> On 31.08.2018 16:57, Miquel Raynal wrote:
> > While 1kB or 1kiB will be parsed correctly, 1k will return the right
> > amount, but the metric suffix will not be escaped once the char
> > pointer updated. Fix this situation by simplifying the move of the
> > endp pointer.  
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> > ---
> >   lib/strto.c | 22 ++++++++++------------
> >   1 file changed, 10 insertions(+), 12 deletions(-)  
> > > diff --git a/lib/strto.c b/lib/strto.c  
> > index 84f8d92d57..502a0153e7 100644
> > --- a/lib/strto.c
> > +++ b/lib/strto.c
> > @@ -97,12 +97,11 @@ unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
> >   	case 'K':
> >   	case 'k':
> >   		result *= 1024;
> > -		if ((*endp)[1] == 'i') {
> > -			if ((*endp)[2] == 'B')
> > -				(*endp) += 3;
> > -			else
> > -				(*endp) += 2;
> > -		}
> > +		(*endp)++;
> > +		if (**endp == 'i')
> > +			(*endp)++;
> > +		if (**endp == 'B')
> > +			(*endp)++;
> >   	}
> >   	return result;
> >   }
> > @@ -122,12 +121,11 @@ unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
> >   	case 'K':
> >   	case 'k':
> >   		result *= 1024;
> > -		if ((*endp)[1] == 'i') {
> > -			if ((*endp)[2] == 'B')
> > -				(*endp) += 3;
> > -			else
> > -				(*endp) += 2;
> > -		}
> > +		(*endp)++;
> > +		if (**endp == 'i')
> > +			(*endp)++;
> > +		if (**endp == 'B')
> > +			(*endp)++;
> >   	}
> >   	return result;
> >   }
> >   
> Even though KiB is not equal to KB in general (at least in Linux
> userspace AFAIK), lets not change this in U-Boot and always use
> KiB and KB as a representation for 1024 (instead of 1000). So:

That's right I did not mentioned it in the commit log. I could
update it to reflect that it is intentional to mix 'k' and 'kiB' as a
representation of '* 12014' (already the case, but being clarified in
the above change).

Thanks,
Miquèl

[U-Boot] [PATCH v7 10/13] cmd: mtd: add 'mtd' command

[Miquel Raynal]

Hi Stefan,

Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 10:59:56 +0200:

> On 31.08.2018 16:57, Miquel Raynal wrote:
> > There should not be a 'nand' command, a 'sf' command and certainly not
> > a new 'spi-nand' command. Write a 'mtd' command instead to manage all
> > MTD devices/partitions at once. This should be the preferred way to
> > access any MTD device.  
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> > Acked-by: Jagan Teki <jagan@openedev.com>
> > ---
> >   cmd/Kconfig          |  10 +-
> >   cmd/Makefile         |   1 +
> >   cmd/mtd.c            | 517 +++++++++++++++++++++++++++++++++++++++++++
> >   drivers/mtd/Makefile |   2 +-
> >   include/mtd.h        |   1 +
> >   5 files changed, 528 insertions(+), 3 deletions(-)
> >   create mode 100644 cmd/mtd.c  
> > > diff --git a/cmd/Kconfig b/cmd/Kconfig  
> > index ef43ed8dda..0778d2ecff 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -847,6 +847,12 @@ config CMD_MMC_SWRITE
> >   	  Enable support for the "mmc swrite" command to write Android sparse
> >   	  images to eMMC.  
> >   > +config CMD_MTD  
> > +	bool "mtd"
> > +	select MTD_PARTITIONS
> > +	help
> > +	  MTD commands support.
> > +
> >   config CMD_NAND
> >   	bool "nand"
> >   	default y if NAND_SUNXI
> > @@ -1671,14 +1677,14 @@ config CMD_MTDPARTS  
> >   >   config MTDIDS_DEFAULT  
> >   	string "Default MTD IDs"
> > -	depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> > +	depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> >   	help
> >   	  Defines a default MTD IDs list for use with MTD partitions in the
> >   	  Linux MTD command line partitions format.  
> >   >   config MTDPARTS_DEFAULT  
> >   	string "Default MTD partition scheme"
> > -	depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> > +	depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> >   	help
> >   	  Defines a default MTD partitioning scheme in the Linux MTD command
> >   	  line partitions format
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 323f1fd2c7..32fd102189 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -90,6 +90,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..32295fe86c
> > --- /dev/null
> > +++ b/cmd/mtd.c
> > @@ -0,0 +1,517 @@
> > +// 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 <command.h>
> > +#include <common.h>
> > +#include <console.h>
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/mtd/partitions.h>
> > +#include <malloc.h>
> > +#include <mapmem.h>
> > +#include <mtd.h>
> > +#include <dm/device.h>
> > +#include <dm/uclass-internal.h>
> > +
> > +#define MTD_NAME_MAX_LEN 20
> > +
> > +static char *old_mtdparts;
> > +
> > +static void mtd_dump_buf(u8 *buf, uint len, uint offset)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 0; i < len; ) {
> > +		printf("0x%08x:\t", offset + 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_parts(struct mtd_info *mtd, int level)
> > +{
> > +	struct mtd_info *part;
> > +	int i;
> > +
> > +	if (list_empty(&mtd->partitions))
> > +		return;
> > +
> > +	list_for_each_entry(part, &mtd->partitions, node) {
> > +		for (i = 0; i < level; i++)
> > +			printf("\t");
> > +		printf("* %s\n", part->name);
> > +		for (i = 0; i < level; i++)
> > +			printf("\t");
> > +		printf("  > Offset: 0x%llx bytes\n", part->offset);
> > +		for (i = 0; i < level; i++)
> > +			printf("\t");
> > +		printf("  > Size: 0x%llx bytes\n", part->size);
> > +
> > +		mtd_show_parts(part, level + 1);
> > +	}
> > +}
> > +
> > +static void mtd_show_device(struct mtd_info *mtd)
> > +{
> > +	/* Device */
> > +	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");
> > +
> > +	/* MTD device information */
> > +	printf("  > type: ");
> > +	switch (mtd->type) {
> > +	case MTD_RAM:
> > +		printf("RAM\n");
> > +		break;
> > +	case MTD_ROM:
> > +		printf("ROM\n");
> > +		break;
> > +	case MTD_NORFLASH:
> > +		printf("NOR flash\n");
> > +		break;
> > +	case MTD_NANDFLASH:
> > +		printf("NAND flash\n");
> > +		break;
> > +	case MTD_DATAFLASH:
> > +		printf("Data flash\n");
> > +		break;
> > +	case MTD_UBIVOLUME:
> > +		printf("UBI volume\n");
> > +		break;
> > +	case MTD_MLCNANDFLASH:
> > +		printf("MLC NAND flash\n");
> > +		break;
> > +	case MTD_ABSENT:
> > +	default:
> > +		printf("Unknown\n");
> > +		break;
> > +	}
> > +
> > +	printf("  > Size: 0x%llx bytes\n", mtd->size);
> > +	printf("  > Block: 0x%x bytes\n", mtd->erasesize);
> > +	printf("  > Min I/O: 0x%x bytes\n", mtd->writesize);
> > +
> > +	if (mtd->oobsize) {
> > +		printf("  > OOB size: %u bytes\n", mtd->oobsize);
> > +		printf("  > OOB available: %u bytes\n", mtd->oobavail);
> > +	}
> > +
> > +	if (mtd->ecc_strength) {
> > +		printf("  > ECC strength: %u bits\n", mtd->ecc_strength);
> > +		printf("  > ECC step size: %u bytes\n", mtd->ecc_step_size);
> > +		printf("  > Bitflip threshold: %u bits\n",
> > +		       mtd->bitflip_threshold);
> > +	}
> > +
> > +	/* MTD partitions, if any */
> > +	mtd_show_parts(mtd, 1);
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_MTD)
> > +static void mtd_probe_uclass_mtd_devs(void)
> > +{
> > +	struct udevice *dev;
> > +	int idx = 0;
> > +
> > +	/* Probe devices with DM compliant drivers */
> > +	while (!uclass_find_device(UCLASS_MTD, idx, &dev) && dev) {
> > +		mtd_probe(dev);
> > +		idx++;
> > +	}
> > +}
> > +#else
> > +static void mtd_probe_uclass_mtd_devs(void) { }
> > +#endif  
> 
> Does it make sense to support this new command without CONFIG_MTD
> enabled? Do you have a use case for this?

Maybe it is a bit misleading and I could add a comment. CONFIG_MTD
depends on CONFIG_DM. Any MTD device could be managed by the 'mtd'
command, but a lot of drivers do not make use of U-Boot DM yet, so
Boris and me thought it would help the acceptance if it could work with
all the devices (with let's say "reduced" functionalities).

Thanks,
Miquèl

[U-Boot] [PATCH v7 11/13] cmd: mtdparts: try to probe the MTD devices as a fallback

[Miquel Raynal]

Hi Stefan,

Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 11:02:43 +0200:

> On 31.08.2018 16:57, Miquel Raynal wrote:
> > Current implementation of mtdparts command errors out if the desired MTD
> > device is not found. Fallback to the new probe function in this case
> > before erroring out.  
> > > This will the save the user the need to call something like 'mtd list'  
> > before mtdparts.  
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> > Acked-by: Jagan Teki <jagan@openedev.com>
> > ---
> >   cmd/mtdparts.c | 16 +++++++++++++---
> >   1 file changed, 13 insertions(+), 3 deletions(-)  
> > > diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c  
> > index f7ed1a0779..a1102c3fc5 100644
> > --- a/cmd/mtdparts.c
> > +++ b/cmd/mtdparts.c
> > @@ -79,6 +79,10 @@
> >   #include <linux/err.h>
> >   #include <linux/mtd/mtd.h>  
> >   > +#if defined(CONFIG_MTD)  
> > +#include <mtd.h>
> > +#endif
> > +
> >   #if defined(CONFIG_CMD_NAND)
> >   #include <linux/mtd/rawnand.h>
> >   #include <nand.h>
> > @@ -307,9 +311,15 @@ static int get_mtd_info(u8 type, u8 num, struct mtd_info **mtd)  
> >   >   	sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(type), num);  
> >   	*mtd = get_mtd_device_nm(mtd_dev);
> > -	if (IS_ERR(*mtd)) {
> > -		printf("Device %s not found!\n", mtd_dev);
> > -		return 1;
> > +	if (IS_ERR_OR_NULL(*mtd)) {
> > +#ifdef CONFIG_MTD
> > +		mtd_probe_devices();
> > +		*mtd = get_mtd_device_nm(mtd_dev);
> > +#endif
> > +		if (IS_ERR_OR_NULL(*mtd)) {
> > +			printf("Device %s not found!\n", mtd_dev);
> > +			return 1;
> > +		}
> >   	}
> >   	put_mtd_device(*mtd);  
> >   >   
> This is most likely the use case for CMD_MTD without MTD, correct?

Not exactly, I think the use case you were looking for is explained in
my previous answer and this #ifdef stands for the opposite situation,
where someone using the 'legacy' commands wants to interact with
devices with DM compliant drivers: the MTD device won't be registered
before (and thus, not accessible) the mtd_probe_device() call.

For other devices the use will have to probe manually the device, eg.
for SPI-NOR: 'sf probe 0'.

> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Thanks,
> Stefan


Thanks,
Miquèl

[U-Boot] [PATCH v7 05/13] cmd: mtdparts: remove mandatory 'mtdparts=' prefix

[Miquel Raynal]

Hi Stefan,

Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 10:50:54 +0200:

> On 31.08.2018 16:57, Miquel Raynal wrote:
> > All U-Boot users must define the mtdparts environment variable with:
> > setenv mtdparts mtdparts=...  
> > > While this may ease the partition declaration job to be passed to  
> > Linux, this is a pure software limitation and forcing this prefix is a
> > complete non-sense. Let the user to declare manually the mtdparts
> > variable without the prefix.  
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> > Acked-by: Jagan Teki <jagan@openedev.com>
> > ---
> >   cmd/mtdparts.c | 17 ++++++-----------
> >   1 file changed, 6 insertions(+), 11 deletions(-)  
> > > diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c  
> > index 2e547894c6..f7ed1a0779 100644
> > --- a/cmd/mtdparts.c
> > +++ b/cmd/mtdparts.c
> > @@ -44,7 +44,7 @@
> >    *
> >    * 'mtdparts' - partition list
> >    *
> > - * mtdparts=mtdparts=<mtd-def>[;<mtd-def>...]
> > + * mtdparts=[mtdparts=]<mtd-def>[;<mtd-def>...]
> >    *
> >    * <mtd-def>  := <mtd-id>:<part-def>[,<part-def>...]
> >    * <mtd-id>   := unique device tag used by linux kernel to find mtd device (mtd->name)
> > @@ -62,11 +62,11 @@
> >    *
> >    * 1 NOR Flash, with 1 single writable partition:
> >    * mtdids=nor0=edb7312-nor
> > - * mtdparts=mtdparts=edb7312-nor:-
> > + * mtdparts=[mtdparts=]edb7312-nor:-
> >    *
> >    * 1 NOR Flash with 2 partitions, 1 NAND with one
> >    * mtdids=nor0=edb7312-nor,nand0=edb7312-nand
> > - * mtdparts=mtdparts=edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
> > + * mtdparts=[mtdparts=]edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
> >    *
> >    */  
> >   > @@ -1099,9 +1099,6 @@ static int generate_mtdparts(char *buf, u32 buflen)  
> >   		return 0;
> >   	}  
> >   > -	strcpy(p, "mtdparts=");  
> > -	p += 9;
> > -
> >   	list_for_each(dentry, &devices) {
> >   		dev = list_entry(dentry, struct mtd_device, link);  
> >   > @@ -1572,11 +1569,9 @@ static int parse_mtdparts(const char *const mtdparts)  
> >   	if (!p)
> >   		p = mtdparts;  
> >   > -	if (strncmp(p, "mtdparts=", 9) != 0) {  
> > -		printf("mtdparts variable doesn't start with 'mtdparts='\n");
> > -		return err;
> > -	}
> > -	p += 9;
> > +	/* Skip the useless prefix, if any */
> > +	if (strncmp(p, "mtdparts=", 9) == 0)
> > +		p += 9;  
> >   >   	while (*p != '\0') {  
> >   		err = 1;
> >   
> I always wondered about this double mtdparts thing but was never brave
> enough to touch it. ;)

If I understand correctly, this double mtdparts thing comes from lazy
people who decided to build the bootargs this way:

setenv mtdparts "mtdparts=mtddev0:-(all)"
setenv bootargs "console=/dev/tty0 [...] ${mtdparts}"

instead of this way:

setenv mtdparts "mtddev0:-(all)"
setenv bootargs "console=/dev/tty0 [...] mtdparts=${mtdparts}"

Which, personally, I find much clearer.

I'm still puzzled by the real use of mtdids, I will check this week if
I didn't broke anything in the way U-Boot and Linux share the
MTD devices and their partitions.

Thanks,
Miquèl

[U-Boot] [PATCH v7 02/13] lib: strto: fix metric suffix parsing in strtoul[l]

[Stefan]

On 03.09.2018 08:49, Miquel Raynal wrote:
> Hi Stefan,
> 
> Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 10:48:25 +0200:
> 
>> On 31.08.2018 16:57, Miquel Raynal wrote:
>>> While 1kB or 1kiB will be parsed correctly, 1k will return the right
>>> amount, but the metric suffix will not be escaped once the char
>>> pointer updated. Fix this situation by simplifying the move of the
>>> endp pointer.
>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>>    lib/strto.c | 22 ++++++++++------------
>>>    1 file changed, 10 insertions(+), 12 deletions(-)
>>>> diff --git a/lib/strto.c b/lib/strto.c
>>> index 84f8d92d57..502a0153e7 100644
>>> --- a/lib/strto.c
>>> +++ b/lib/strto.c
>>> @@ -97,12 +97,11 @@ unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
>>>    	case 'K':
>>>    	case 'k':
>>>    		result *= 1024;
>>> -		if ((*endp)[1] == 'i') {
>>> -			if ((*endp)[2] == 'B')
>>> -				(*endp) += 3;
>>> -			else
>>> -				(*endp) += 2;
>>> -		}
>>> +		(*endp)++;
>>> +		if (**endp == 'i')
>>> +			(*endp)++;
>>> +		if (**endp == 'B')
>>> +			(*endp)++;
>>>    	}
>>>    	return result;
>>>    }
>>> @@ -122,12 +121,11 @@ unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
>>>    	case 'K':
>>>    	case 'k':
>>>    		result *= 1024;
>>> -		if ((*endp)[1] == 'i') {
>>> -			if ((*endp)[2] == 'B')
>>> -				(*endp) += 3;
>>> -			else
>>> -				(*endp) += 2;
>>> -		}
>>> +		(*endp)++;
>>> +		if (**endp == 'i')
>>> +			(*endp)++;
>>> +		if (**endp == 'B')
>>> +			(*endp)++;
>>>    	}
>>>    	return result;
>>>    }
>>>    
>> Even though KiB is not equal to KB in general (at least in Linux
>> userspace AFAIK), lets not change this in U-Boot and always use
>> KiB and KB as a representation for 1024 (instead of 1000). So:
> 
> That's right I did not mentioned it in the commit log. I could
> update it to reflect that it is intentional to mix 'k' and 'kiB' as a
> representation of '* 12014' (already the case, but being clarified in
> the above change).

If you will send a new version of these patches, then this would be
great to clarify this usage.

Thanks,
Stefan

[U-Boot] [PATCH v7 10/13] cmd: mtd: add 'mtd' command

[Stefan]

On 03.09.2018 08:54, Miquel Raynal wrote:
> Hi Stefan,
> 
> Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 10:59:56 +0200:
> 
>> On 31.08.2018 16:57, Miquel Raynal wrote:
>>> There should not be a 'nand' command, a 'sf' command and certainly not
>>> a new 'spi-nand' command. Write a 'mtd' command instead to manage all
>>> MTD devices/partitions at once. This should be the preferred way to
>>> access any MTD device.
>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> Acked-by: Jagan Teki <jagan@openedev.com>
>>> ---
>>>    cmd/Kconfig          |  10 +-
>>>    cmd/Makefile         |   1 +
>>>    cmd/mtd.c            | 517 +++++++++++++++++++++++++++++++++++++++++++
>>>    drivers/mtd/Makefile |   2 +-
>>>    include/mtd.h        |   1 +
>>>    5 files changed, 528 insertions(+), 3 deletions(-)
>>>    create mode 100644 cmd/mtd.c
>>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>> index ef43ed8dda..0778d2ecff 100644
>>> --- a/cmd/Kconfig
>>> +++ b/cmd/Kconfig
>>> @@ -847,6 +847,12 @@ config CMD_MMC_SWRITE
>>>    	  Enable support for the "mmc swrite" command to write Android sparse
>>>    	  images to eMMC.
>>>    > +config CMD_MTD
>>> +	bool "mtd"
>>> +	select MTD_PARTITIONS
>>> +	help
>>> +	  MTD commands support.
>>> +
>>>    config CMD_NAND
>>>    	bool "nand"
>>>    	default y if NAND_SUNXI
>>> @@ -1671,14 +1677,14 @@ config CMD_MTDPARTS
>>>    >   config MTDIDS_DEFAULT
>>>    	string "Default MTD IDs"
>>> -	depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>>> +	depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>>>    	help
>>>    	  Defines a default MTD IDs list for use with MTD partitions in the
>>>    	  Linux MTD command line partitions format.
>>>    >   config MTDPARTS_DEFAULT
>>>    	string "Default MTD partition scheme"
>>> -	depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>>> +	depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>>>    	help
>>>    	  Defines a default MTD partitioning scheme in the Linux MTD command
>>>    	  line partitions format
>>> diff --git a/cmd/Makefile b/cmd/Makefile
>>> index 323f1fd2c7..32fd102189 100644
>>> --- a/cmd/Makefile
>>> +++ b/cmd/Makefile
>>> @@ -90,6 +90,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..32295fe86c
>>> --- /dev/null
>>> +++ b/cmd/mtd.c
>>> @@ -0,0 +1,517 @@
>>> +// 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 <command.h>
>>> +#include <common.h>
>>> +#include <console.h>
>>> +#include <linux/mtd/mtd.h>
>>> +#include <linux/mtd/partitions.h>
>>> +#include <malloc.h>
>>> +#include <mapmem.h>
>>> +#include <mtd.h>
>>> +#include <dm/device.h>
>>> +#include <dm/uclass-internal.h>
>>> +
>>> +#define MTD_NAME_MAX_LEN 20
>>> +
>>> +static char *old_mtdparts;
>>> +
>>> +static void mtd_dump_buf(u8 *buf, uint len, uint offset)
>>> +{
>>> +	int i, j;
>>> +
>>> +	for (i = 0; i < len; ) {
>>> +		printf("0x%08x:\t", offset + 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_parts(struct mtd_info *mtd, int level)
>>> +{
>>> +	struct mtd_info *part;
>>> +	int i;
>>> +
>>> +	if (list_empty(&mtd->partitions))
>>> +		return;
>>> +
>>> +	list_for_each_entry(part, &mtd->partitions, node) {
>>> +		for (i = 0; i < level; i++)
>>> +			printf("\t");
>>> +		printf("* %s\n", part->name);
>>> +		for (i = 0; i < level; i++)
>>> +			printf("\t");
>>> +		printf("  > Offset: 0x%llx bytes\n", part->offset);
>>> +		for (i = 0; i < level; i++)
>>> +			printf("\t");
>>> +		printf("  > Size: 0x%llx bytes\n", part->size);
>>> +
>>> +		mtd_show_parts(part, level + 1);
>>> +	}
>>> +}
>>> +
>>> +static void mtd_show_device(struct mtd_info *mtd)
>>> +{
>>> +	/* Device */
>>> +	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");
>>> +
>>> +	/* MTD device information */
>>> +	printf("  > type: ");
>>> +	switch (mtd->type) {
>>> +	case MTD_RAM:
>>> +		printf("RAM\n");
>>> +		break;
>>> +	case MTD_ROM:
>>> +		printf("ROM\n");
>>> +		break;
>>> +	case MTD_NORFLASH:
>>> +		printf("NOR flash\n");
>>> +		break;
>>> +	case MTD_NANDFLASH:
>>> +		printf("NAND flash\n");
>>> +		break;
>>> +	case MTD_DATAFLASH:
>>> +		printf("Data flash\n");
>>> +		break;
>>> +	case MTD_UBIVOLUME:
>>> +		printf("UBI volume\n");
>>> +		break;
>>> +	case MTD_MLCNANDFLASH:
>>> +		printf("MLC NAND flash\n");
>>> +		break;
>>> +	case MTD_ABSENT:
>>> +	default:
>>> +		printf("Unknown\n");
>>> +		break;
>>> +	}
>>> +
>>> +	printf("  > Size: 0x%llx bytes\n", mtd->size);
>>> +	printf("  > Block: 0x%x bytes\n", mtd->erasesize);
>>> +	printf("  > Min I/O: 0x%x bytes\n", mtd->writesize);
>>> +
>>> +	if (mtd->oobsize) {
>>> +		printf("  > OOB size: %u bytes\n", mtd->oobsize);
>>> +		printf("  > OOB available: %u bytes\n", mtd->oobavail);
>>> +	}
>>> +
>>> +	if (mtd->ecc_strength) {
>>> +		printf("  > ECC strength: %u bits\n", mtd->ecc_strength);
>>> +		printf("  > ECC step size: %u bytes\n", mtd->ecc_step_size);
>>> +		printf("  > Bitflip threshold: %u bits\n",
>>> +		       mtd->bitflip_threshold);
>>> +	}
>>> +
>>> +	/* MTD partitions, if any */
>>> +	mtd_show_parts(mtd, 1);
>>> +}
>>> +
>>> +#if IS_ENABLED(CONFIG_MTD)
>>> +static void mtd_probe_uclass_mtd_devs(void)
>>> +{
>>> +	struct udevice *dev;
>>> +	int idx = 0;
>>> +
>>> +	/* Probe devices with DM compliant drivers */
>>> +	while (!uclass_find_device(UCLASS_MTD, idx, &dev) && dev) {
>>> +		mtd_probe(dev);
>>> +		idx++;
>>> +	}
>>> +}
>>> +#else
>>> +static void mtd_probe_uclass_mtd_devs(void) { }
>>> +#endif
>>
>> Does it make sense to support this new command without CONFIG_MTD
>> enabled? Do you have a use case for this?
> 
> Maybe it is a bit misleading and I could add a comment. CONFIG_MTD
> depends on CONFIG_DM. Any MTD device could be managed by the 'mtd'
> command, but a lot of drivers do not make use of U-Boot DM yet, so
> Boris and me thought it would help the acceptance if it could work with
> all the devices (with let's say "reduced" functionalities).

Makes perfect sense this way, thank. I see no need to add a comment
in a potential new patch version.

Thanks,
Stefan

[U-Boot] [PATCH v7 10/13] cmd: mtd: add 'mtd' command

[Jagan Teki]

On Mon, Sep 3, 2018 at 12:24 PM, Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> Hi Stefan,
>
> Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 10:59:56 +0200:
>
>> On 31.08.2018 16:57, Miquel Raynal wrote:
>> > There should not be a 'nand' command, a 'sf' command and certainly not
>> > a new 'spi-nand' command. Write a 'mtd' command instead to manage all
>> > MTD devices/partitions at once. This should be the preferred way to
>> > access any MTD device.
>> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> > Acked-by: Jagan Teki <jagan@openedev.com>
>> > ---
>> >   cmd/Kconfig          |  10 +-
>> >   cmd/Makefile         |   1 +
>> >   cmd/mtd.c            | 517 +++++++++++++++++++++++++++++++++++++++++++
>> >   drivers/mtd/Makefile |   2 +-
>> >   include/mtd.h        |   1 +
>> >   5 files changed, 528 insertions(+), 3 deletions(-)
>> >   create mode 100644 cmd/mtd.c
>> > > diff --git a/cmd/Kconfig b/cmd/Kconfig
>> > index ef43ed8dda..0778d2ecff 100644
>> > --- a/cmd/Kconfig
>> > +++ b/cmd/Kconfig
>> > @@ -847,6 +847,12 @@ config CMD_MMC_SWRITE
>> >       Enable support for the "mmc swrite" command to write Android sparse
>> >       images to eMMC.
>> >   > +config CMD_MTD
>> > +   bool "mtd"
>> > +   select MTD_PARTITIONS
>> > +   help
>> > +     MTD commands support.
>> > +
>> >   config CMD_NAND
>> >     bool "nand"
>> >     default y if NAND_SUNXI
>> > @@ -1671,14 +1677,14 @@ config CMD_MTDPARTS
>> >   >   config MTDIDS_DEFAULT
>> >     string "Default MTD IDs"
>> > -   depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>> > +   depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>> >     help
>> >       Defines a default MTD IDs list for use with MTD partitions in the
>> >       Linux MTD command line partitions format.
>> >   >   config MTDPARTS_DEFAULT
>> >     string "Default MTD partition scheme"
>> > -   depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>> > +   depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
>> >     help
>> >       Defines a default MTD partitioning scheme in the Linux MTD command
>> >       line partitions format
>> > diff --git a/cmd/Makefile b/cmd/Makefile
>> > index 323f1fd2c7..32fd102189 100644
>> > --- a/cmd/Makefile
>> > +++ b/cmd/Makefile
>> > @@ -90,6 +90,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..32295fe86c
>> > --- /dev/null
>> > +++ b/cmd/mtd.c
>> > @@ -0,0 +1,517 @@
>> > +// 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 <command.h>
>> > +#include <common.h>
>> > +#include <console.h>
>> > +#include <linux/mtd/mtd.h>
>> > +#include <linux/mtd/partitions.h>
>> > +#include <malloc.h>
>> > +#include <mapmem.h>
>> > +#include <mtd.h>
>> > +#include <dm/device.h>
>> > +#include <dm/uclass-internal.h>
>> > +
>> > +#define MTD_NAME_MAX_LEN 20
>> > +
>> > +static char *old_mtdparts;
>> > +
>> > +static void mtd_dump_buf(u8 *buf, uint len, uint offset)
>> > +{
>> > +   int i, j;
>> > +
>> > +   for (i = 0; i < len; ) {
>> > +           printf("0x%08x:\t", offset + 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_parts(struct mtd_info *mtd, int level)
>> > +{
>> > +   struct mtd_info *part;
>> > +   int i;
>> > +
>> > +   if (list_empty(&mtd->partitions))
>> > +           return;
>> > +
>> > +   list_for_each_entry(part, &mtd->partitions, node) {
>> > +           for (i = 0; i < level; i++)
>> > +                   printf("\t");
>> > +           printf("* %s\n", part->name);
>> > +           for (i = 0; i < level; i++)
>> > +                   printf("\t");
>> > +           printf("  > Offset: 0x%llx bytes\n", part->offset);
>> > +           for (i = 0; i < level; i++)
>> > +                   printf("\t");
>> > +           printf("  > Size: 0x%llx bytes\n", part->size);
>> > +
>> > +           mtd_show_parts(part, level + 1);
>> > +   }
>> > +}
>> > +
>> > +static void mtd_show_device(struct mtd_info *mtd)
>> > +{
>> > +   /* Device */
>> > +   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");
>> > +
>> > +   /* MTD device information */
>> > +   printf("  > type: ");
>> > +   switch (mtd->type) {
>> > +   case MTD_RAM:
>> > +           printf("RAM\n");
>> > +           break;
>> > +   case MTD_ROM:
>> > +           printf("ROM\n");
>> > +           break;
>> > +   case MTD_NORFLASH:
>> > +           printf("NOR flash\n");
>> > +           break;
>> > +   case MTD_NANDFLASH:
>> > +           printf("NAND flash\n");
>> > +           break;
>> > +   case MTD_DATAFLASH:
>> > +           printf("Data flash\n");
>> > +           break;
>> > +   case MTD_UBIVOLUME:
>> > +           printf("UBI volume\n");
>> > +           break;
>> > +   case MTD_MLCNANDFLASH:
>> > +           printf("MLC NAND flash\n");
>> > +           break;
>> > +   case MTD_ABSENT:
>> > +   default:
>> > +           printf("Unknown\n");
>> > +           break;
>> > +   }
>> > +
>> > +   printf("  > Size: 0x%llx bytes\n", mtd->size);
>> > +   printf("  > Block: 0x%x bytes\n", mtd->erasesize);
>> > +   printf("  > Min I/O: 0x%x bytes\n", mtd->writesize);
>> > +
>> > +   if (mtd->oobsize) {
>> > +           printf("  > OOB size: %u bytes\n", mtd->oobsize);
>> > +           printf("  > OOB available: %u bytes\n", mtd->oobavail);
>> > +   }
>> > +
>> > +   if (mtd->ecc_strength) {
>> > +           printf("  > ECC strength: %u bits\n", mtd->ecc_strength);
>> > +           printf("  > ECC step size: %u bytes\n", mtd->ecc_step_size);
>> > +           printf("  > Bitflip threshold: %u bits\n",
>> > +                  mtd->bitflip_threshold);
>> > +   }
>> > +
>> > +   /* MTD partitions, if any */
>> > +   mtd_show_parts(mtd, 1);
>> > +}
>> > +
>> > +#if IS_ENABLED(CONFIG_MTD)
>> > +static void mtd_probe_uclass_mtd_devs(void)
>> > +{
>> > +   struct udevice *dev;
>> > +   int idx = 0;
>> > +
>> > +   /* Probe devices with DM compliant drivers */
>> > +   while (!uclass_find_device(UCLASS_MTD, idx, &dev) && dev) {
>> > +           mtd_probe(dev);
>> > +           idx++;
>> > +   }
>> > +}
>> > +#else
>> > +static void mtd_probe_uclass_mtd_devs(void) { }
>> > +#endif
>>
>> Does it make sense to support this new command without CONFIG_MTD
>> enabled? Do you have a use case for this?
>
> Maybe it is a bit misleading and I could add a comment. CONFIG_MTD
> depends on CONFIG_DM. Any MTD device could be managed by the 'mtd'
> command, but a lot of drivers do not make use of U-Boot DM yet, so
> Boris and me thought it would help the acceptance if it could work with
> all the devices (with let's say "reduced" functionalities).

I think giving convince way to use mtd cmd for non-dm code may not
good for long run, because non-dm drivers never convert (or may take
long time) to dm and also there may be a possibility of adding #ifndef
CONFIG_MTD in mtd  code for the sake of non-dm to work with dm code.

[U-Boot] [PATCH v7 10/13] cmd: mtd: add 'mtd' command

[Miquel Raynal]

Hi Jagan,

Jagan Teki <jagan@amarulasolutions.com> wrote on Mon, 3 Sep 2018
13:34:21 +0530:

> On Mon, Sep 3, 2018 at 12:24 PM, Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > Hi Stefan,
> >
> > Stefan Roese <sr@denx.de> wrote on Sat, 1 Sep 2018 10:59:56 +0200:
> >  
> >> On 31.08.2018 16:57, Miquel Raynal wrote:  
> >> > There should not be a 'nand' command, a 'sf' command and certainly not
> >> > a new 'spi-nand' command. Write a 'mtd' command instead to manage all
> >> > MTD devices/partitions at once. This should be the preferred way to
> >> > access any MTD device.  
> >> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> >> > Acked-by: Jagan Teki <jagan@openedev.com>
> >> > ---
> >> >   cmd/Kconfig          |  10 +-
> >> >   cmd/Makefile         |   1 +
> >> >   cmd/mtd.c            | 517 +++++++++++++++++++++++++++++++++++++++++++
> >> >   drivers/mtd/Makefile |   2 +-
> >> >   include/mtd.h        |   1 +
> >> >   5 files changed, 528 insertions(+), 3 deletions(-)
> >> >   create mode 100644 cmd/mtd.c  
> >> > > diff --git a/cmd/Kconfig b/cmd/Kconfig  
> >> > index ef43ed8dda..0778d2ecff 100644
> >> > --- a/cmd/Kconfig
> >> > +++ b/cmd/Kconfig
> >> > @@ -847,6 +847,12 @@ config CMD_MMC_SWRITE
> >> >       Enable support for the "mmc swrite" command to write Android sparse
> >> >       images to eMMC.  
> >> >   > +config CMD_MTD  
> >> > +   bool "mtd"
> >> > +   select MTD_PARTITIONS
> >> > +   help
> >> > +     MTD commands support.
> >> > +
> >> >   config CMD_NAND
> >> >     bool "nand"
> >> >     default y if NAND_SUNXI
> >> > @@ -1671,14 +1677,14 @@ config CMD_MTDPARTS  
> >> >   >   config MTDIDS_DEFAULT  
> >> >     string "Default MTD IDs"
> >> > -   depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> >> > +   depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> >> >     help
> >> >       Defines a default MTD IDs list for use with MTD partitions in the
> >> >       Linux MTD command line partitions format.  
> >> >   >   config MTDPARTS_DEFAULT  
> >> >     string "Default MTD partition scheme"
> >> > -   depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> >> > +   depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> >> >     help
> >> >       Defines a default MTD partitioning scheme in the Linux MTD command
> >> >       line partitions format
> >> > diff --git a/cmd/Makefile b/cmd/Makefile
> >> > index 323f1fd2c7..32fd102189 100644
> >> > --- a/cmd/Makefile
> >> > +++ b/cmd/Makefile
> >> > @@ -90,6 +90,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..32295fe86c
> >> > --- /dev/null
> >> > +++ b/cmd/mtd.c
> >> > @@ -0,0 +1,517 @@
> >> > +// 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 <command.h>
> >> > +#include <common.h>
> >> > +#include <console.h>
> >> > +#include <linux/mtd/mtd.h>
> >> > +#include <linux/mtd/partitions.h>
> >> > +#include <malloc.h>
> >> > +#include <mapmem.h>
> >> > +#include <mtd.h>
> >> > +#include <dm/device.h>
> >> > +#include <dm/uclass-internal.h>
> >> > +
> >> > +#define MTD_NAME_MAX_LEN 20
> >> > +
> >> > +static char *old_mtdparts;
> >> > +
> >> > +static void mtd_dump_buf(u8 *buf, uint len, uint offset)
> >> > +{
> >> > +   int i, j;
> >> > +
> >> > +   for (i = 0; i < len; ) {
> >> > +           printf("0x%08x:\t", offset + 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_parts(struct mtd_info *mtd, int level)
> >> > +{
> >> > +   struct mtd_info *part;
> >> > +   int i;
> >> > +
> >> > +   if (list_empty(&mtd->partitions))
> >> > +           return;
> >> > +
> >> > +   list_for_each_entry(part, &mtd->partitions, node) {
> >> > +           for (i = 0; i < level; i++)
> >> > +                   printf("\t");
> >> > +           printf("* %s\n", part->name);
> >> > +           for (i = 0; i < level; i++)
> >> > +                   printf("\t");
> >> > +           printf("  > Offset: 0x%llx bytes\n", part->offset);
> >> > +           for (i = 0; i < level; i++)
> >> > +                   printf("\t");
> >> > +           printf("  > Size: 0x%llx bytes\n", part->size);
> >> > +
> >> > +           mtd_show_parts(part, level + 1);
> >> > +   }
> >> > +}
> >> > +
> >> > +static void mtd_show_device(struct mtd_info *mtd)
> >> > +{
> >> > +   /* Device */
> >> > +   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");
> >> > +
> >> > +   /* MTD device information */
> >> > +   printf("  > type: ");
> >> > +   switch (mtd->type) {
> >> > +   case MTD_RAM:
> >> > +           printf("RAM\n");
> >> > +           break;
> >> > +   case MTD_ROM:
> >> > +           printf("ROM\n");
> >> > +           break;
> >> > +   case MTD_NORFLASH:
> >> > +           printf("NOR flash\n");
> >> > +           break;
> >> > +   case MTD_NANDFLASH:
> >> > +           printf("NAND flash\n");
> >> > +           break;
> >> > +   case MTD_DATAFLASH:
> >> > +           printf("Data flash\n");
> >> > +           break;
> >> > +   case MTD_UBIVOLUME:
> >> > +           printf("UBI volume\n");
> >> > +           break;
> >> > +   case MTD_MLCNANDFLASH:
> >> > +           printf("MLC NAND flash\n");
> >> > +           break;
> >> > +   case MTD_ABSENT:
> >> > +   default:
> >> > +           printf("Unknown\n");
> >> > +           break;
> >> > +   }
> >> > +
> >> > +   printf("  > Size: 0x%llx bytes\n", mtd->size);
> >> > +   printf("  > Block: 0x%x bytes\n", mtd->erasesize);
> >> > +   printf("  > Min I/O: 0x%x bytes\n", mtd->writesize);
> >> > +
> >> > +   if (mtd->oobsize) {
> >> > +           printf("  > OOB size: %u bytes\n", mtd->oobsize);
> >> > +           printf("  > OOB available: %u bytes\n", mtd->oobavail);
> >> > +   }
> >> > +
> >> > +   if (mtd->ecc_strength) {
> >> > +           printf("  > ECC strength: %u bits\n", mtd->ecc_strength);
> >> > +           printf("  > ECC step size: %u bytes\n", mtd->ecc_step_size);
> >> > +           printf("  > Bitflip threshold: %u bits\n",
> >> > +                  mtd->bitflip_threshold);
> >> > +   }
> >> > +
> >> > +   /* MTD partitions, if any */
> >> > +   mtd_show_parts(mtd, 1);
> >> > +}
> >> > +
> >> > +#if IS_ENABLED(CONFIG_MTD)
> >> > +static void mtd_probe_uclass_mtd_devs(void)
> >> > +{
> >> > +   struct udevice *dev;
> >> > +   int idx = 0;
> >> > +
> >> > +   /* Probe devices with DM compliant drivers */
> >> > +   while (!uclass_find_device(UCLASS_MTD, idx, &dev) && dev) {
> >> > +           mtd_probe(dev);
> >> > +           idx++;
> >> > +   }
> >> > +}
> >> > +#else
> >> > +static void mtd_probe_uclass_mtd_devs(void) { }
> >> > +#endif  
> >>
> >> Does it make sense to support this new command without CONFIG_MTD
> >> enabled? Do you have a use case for this?  
> >
> > Maybe it is a bit misleading and I could add a comment. CONFIG_MTD
> > depends on CONFIG_DM. Any MTD device could be managed by the 'mtd'
> > command, but a lot of drivers do not make use of U-Boot DM yet, so
> > Boris and me thought it would help the acceptance if it could work with
> > all the devices (with let's say "reduced" functionalities).  
> 
> I think giving convince way to use mtd cmd for non-dm code may not
> good for long run, because non-dm drivers never convert (or may take
> long time) to dm and also there may be a possibility of adding #ifndef
> CONFIG_MTD in mtd  code for the sake of non-dm to work with dm code.

Indeed, that's what is done here.

Thanks,
Miquèl