SPI-NAND support (third batch)

setenv mtdparts 'spi-nand0:1m(foo),-(bar)'
setenv mtdids 'spi-nand0=spi0.0' # spi0.0 is Linux MTD name for this device
ubi part bar # create a static UBI volume in the bar partition
mtd list # show the current MTD devices/partitions

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 <[email protected]>

Acked-by Jagan Teki <[email protected]>

Reviewed-by Stefan Roese <[email protected]>

Reviewed-by Boris Brezillon <[email protected]>

---

include/mtd.h +0 -0

  /*
   * Get mtd_info structure of the dev, which is stored as uclass private.
   *

@@ -20,5 +23,18 @@ static inline struct mtd_info *mtd_get_info(struct udevice *dev)
  }
 
  int mtd_probe(struct udevice *dev);

Quoted 9 lines

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.

Quoted 1 line

Signed-off-by Miquel Raynal <[email protected]>

Acked-by Jagan Teki <[email protected]>

Reviewed-by Stefan Roese <[email protected]>

Reviewed-by Boris Brezillon <[email protected]>

---

[snip]

include/mtd.h line 11

Quoted 10 lines

Quoted 1 line

static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size,

include/mtd.h +2 -0

index 6e6da3002f..011f26b3e1 100644
@@ -8,6 +8,9 @@  
  >   #include <linux/mtd/mtd.h>
  > +struct udevice;  
+
+#if defined(CONFIG_DM)  

it should be CONFIG_MTD.

include/mtd.h static inline struct mtd_info *mtd_get_info(struct udevice *dev) · line 24

Quoted 7 lines

include/mtd.h +0 -0

  /*
   * Get mtd_info structure of the dev, which is stored as uclass private.
   *

@@ -20,5 +23,18 @@ static inline struct mtd_info *mtd_get_info(struct udevice *dev)
  }  
  >   int mtd_probe(struct udevice *dev);  

[...]

there is not caller for this in non-dm, better this block empty is it?

Let me know so-that I can do that and apply.

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 <[email protected]>

Acked-by Jagan Teki <[email protected]>

Reviewed-by Stefan Roese <[email protected]>

Reviewed-by Boris Brezillon <[email protected]>

---
 cmd/Kconfig             |  10 +-
 cmd/Makefile            |   1 +
 cmd/mtd.c               | 473 ++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/Makefile    |   2 +-
 drivers/mtd/mtd_uboot.c | 161 +++++++++++++-
 include/mtd.h           |  16 ++
 6 files changed, 659 insertions(+), 4 deletions(-)
 create mode 100644 cmd/mtd.c

cmd/Kconfig +8 -2

index 13d4c991bf..c9be85989c 100644
@@ -864,6 +864,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

@@ -1697,14 +1703,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

cmd/Makefile +1 -0

index a61fab6583..3ba65d4d93 100644
@@ -92,6 +92,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

cmd/mtd.c new file +473 -0

new file mode 100644
index 0000000000..6142223984
@@ -0,0 +1,473 @@
+// 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 <[email protected]>
+ */
+
+#include <command.h>
+#include <common.h>
+#include <console.h>
+#include <malloc.h>
+#include <mapmem.h>
+#include <mtd.h>
+
+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 void mtd_dump_buf(const 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_dump_device_buf(struct mtd_info *mtd, u64 start_off,
+                               const u8 *buf, u64 len, bool woob)
+{
+       bool has_pages = mtd->type == MTD_NANDFLASH ||
+               mtd->type == MTD_MLCNANDFLASH;
+       int npages = mtd_len_to_pages(mtd, len);
+       uint page;
+
+       if (has_pages) {
+               for (page = 0; page < npages; page++) {
+                       u64 data_off = page * mtd->writesize;
+
+                       printf("\nDump %d data bytes from 0x%08llx:\n",
+                              mtd->writesize, start_off + data_off);
+                       mtd_dump_buf(&buf[data_off],
+                                    mtd->writesize, start_off + data_off);
+
+                       if (woob) {
+                               u64 oob_off = page * mtd->oobsize;
+
+                               printf("Dump %d OOB bytes from page at 0x%08llx:\n",
+                                      mtd->oobsize, start_off + data_off);
+                               mtd_dump_buf(&buf[len + oob_off],
+                                            mtd->oobsize, 0);
+                       }
+               }
+       } else {
+               printf("\nDump %lld data bytes from 0x%llx:\n",
+                      len, start_off);
+               mtd_dump_buf(buf, len, start_off);
+       }
+}
+
+static void mtd_show_parts(struct mtd_info *mtd, int level)
+{
+       struct mtd_info *part;
+       int i;
+
+       list_for_each_entry(part, &mtd->partitions, node) {
+               for (i = 0; i < level; i++)
+                       printf("\t");
+               printf("  - 0x%012llx-0x%012llx : \"%s\"\n",
+                      part->offset, part->offset + part->size, part->name);
+
+               mtd_show_parts(part, level + 1);
+       }
+}
+
+static void mtd_show_device(struct mtd_info *mtd)
+{
+       /* Device */
+       printf("* %s\n", mtd->name);
+#if defined(CONFIG_DM)
+       if (mtd->dev) {
+               printf("  - device: %s\n", mtd->dev->name);
+               printf("  - parent: %s\n", mtd->dev->parent->name);
+               printf("  - driver: %s\n", mtd->dev->driver->name);
+       }
+#endif
+
+       /* 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("  - block size: 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);
+       }
+
+       printf("  - 0x%012llx-0x%012llx : \"%s\"\n",
+              mtd->offset, mtd->offset + mtd->size, mtd->name);
+
+       /* MTD partitions, if any */
+       mtd_show_parts(mtd, 1);
+}
+
+/* Logic taken from fs/ubifs/recovery.c:is_empty() */
+static bool mtd_oob_write_is_empty(struct mtd_oob_ops *op)
+{
+       int i;
+
+       for (i = 0; i < op->len; i++)
+               if (op->datbuf[i] != 0xff)
+                       return false;
+
+       for (i = 0; i < op->ooblen; i++)
+               if (op->oobbuf[i] != 0xff)
+                       return false;
+
+       return true;
+}
+
+static int do_mtd_list(void)
+{
+       struct mtd_info *mtd;
+       int dev_nb = 0;
+
+       /* Ensure all devices (and their partitions) are probed */
+       mtd_probe_devices();
+
+       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 mtd_special_write_oob(struct mtd_info *mtd, u64 off,
+                                struct mtd_oob_ops *io_op,
+                                bool write_empty_pages, bool woob)
+{
+       int ret = 0;
+
+       /*
+        * By default, do not write an empty page.
+        * Skip it by simulating a successful write.
+        */
+       if (!write_empty_pages && mtd_oob_write_is_empty(io_op)) {
+               io_op->retlen = mtd->writesize;
+               io_op->oobretlen = woob ? mtd->oobsize : 0;
+       } else {
+               ret = mtd_write_oob(mtd, off, io_op);
+       }
+
+       return ret;
+}
+
+static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+       struct mtd_info *mtd;
+       const char *cmd;
+       char *mtd_name;
+
+       /* 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;
+
+       mtd_name = argv[2];
+       mtd_probe_devices();
+       mtd = get_mtd_device_nm(mtd_name);
+       if (IS_ERR_OR_NULL(mtd)) {
+               printf("MTD device %s not found, ret %ld\n",
+                      mtd_name, 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)) {
+               bool has_pages = mtd->type == MTD_NANDFLASH ||
+                                mtd->type == MTD_MLCNANDFLASH;
+               bool dump, read, raw, woob, write_empty_pages;
+               struct mtd_oob_ops io_op = {};
+               uint user_addr = 0, npages;
+               u64 start_off, off, len, remaining, default_len;
+               u32 oob_len;
+               u8 *buf;
+               int ret;
+
+               dump = !strncmp(cmd, "dump", 4);
+               read = dump || !strncmp(cmd, "read", 4);
+               raw = strstr(cmd, ".raw");
+               woob = strstr(cmd, ".oob");
+               write_empty_pages = !has_pages || strstr(cmd, ".dontskipff");
+
+               if (!dump) {
+                       if (!argc)
+                               return CMD_RET_USAGE;
+
+                       user_addr = simple_strtoul(argv[0], NULL, 16);
+                       argc--;
+                       argv++;
+               }
+
+               start_off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
+               if (!mtd_is_aligned_with_min_io_size(mtd, start_off)) {
+                       printf("Offset not aligned with a page (0x%x)\n",
+                              mtd->writesize);
+                       return CMD_RET_FAILURE;
+               }
+
+               default_len = dump ? mtd->writesize : mtd->size;
+               len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) :
+                                default_len;
+               if (!mtd_is_aligned_with_min_io_size(mtd, len)) {
+                       len = round_up(len, mtd->writesize);
+                       printf("Size not on a page boundary (0x%x), rounding to 0x%llx\n",
+                              mtd->writesize, len);
+               }
+
+               remaining = len;
+               npages = mtd_len_to_pages(mtd, len);
+               oob_len = woob ? npages * mtd->oobsize : 0;
+
+               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;
+               }
+
+               if (has_pages)
+                       printf("%s %lld byte(s) (%d page(s)) at offset 0x%08llx%s%s%s\n",
+                              read ? "Reading" : "Writing", len, npages, start_off,
+                              raw ? " [raw]" : "", woob ? " [oob]" : "",
+                              !read && write_empty_pages ? " [dontskipff]" : "");
+               else
+                       printf("%s %lld byte(s) at offset 0x%08llx\n",
+                              read ? "Reading" : "Writing", len, start_off);
+
+               io_op.mode = raw ? MTD_OPS_RAW : MTD_OPS_AUTO_OOB;
+               io_op.len = has_pages ? mtd->writesize : len;
+               io_op.ooblen = woob ? mtd->oobsize : 0;
+               io_op.datbuf = buf;
+               io_op.oobbuf = woob ? &buf[len] : NULL;
+
+               /* Search for the first good block after the given offset */
+               off = start_off;
+               while (mtd_block_isbad(mtd, off))
+                       off += mtd->erasesize;
+
+               /* Loop over the pages to do the actual read/write */
+               while (remaining) {
+                       /* Skip the block if it is bad */
+                       if (mtd_is_aligned_with_block_size(mtd, off) &&
+                           mtd_block_isbad(mtd, off)) {
+                               off += mtd->erasesize;
+                               continue;
+                       }
+
+                       if (read)
+                               ret = mtd_read_oob(mtd, off, &io_op);
+                       else
+                               ret = mtd_special_write_oob(mtd, off, &io_op,
+                                                           write_empty_pages,
+                                                           woob);
+
+                       if (ret) {
+                               printf("Failure while %s at offset 0x%llx\n",
+                                      read ? "reading" : "writing", off);
+                               return CMD_RET_FAILURE;
+                       }
+
+                       off += io_op.retlen;
+                       remaining -= io_op.retlen;
+                       io_op.datbuf += io_op.retlen;
+                       io_op.oobbuf += io_op.oobretlen;
+               }
+
+               if (!ret && dump)
+                       mtd_dump_device_buf(mtd, start_off, buf, len, woob);
+
+               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;
+               int ret;
+
+               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 a 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;
+               }
+
+               if (ret && ret != -EIO)
+                       return CMD_RET_FAILURE;
+       } else if (!strcmp(cmd, "bad")) {
+               loff_t off;
+
+               if (!mtd_can_have_bb(mtd)) {
+                       printf("Only NAND-based devices can have bad blocks\n");
+                       return CMD_RET_SUCCESS;
+               }
+
+               printf("MTD device %s bad blocks list:\n", mtd->name);
+               for (off = 0; off < mtd->size; off += mtd->erasesize)
+                       if (mtd_block_isbad(mtd, off))
+                               printf("\t0x%08llx\n", off);
+       } else {
+               return CMD_RET_USAGE;
+       }
+
+       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][.dontskipff]    <name> <addr> [<off> [<size>]]\n"
+       "mtd erase[.dontskipbad]               <name>        [<off> [<size>]]\n"
+       "\n"
+       "Specific functions:\n"
+       "mtd bad                               <name>\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"
+       "\n"
+       "The .dontskipff option forces writing empty pages, don't use it if unsure.\n"
+#endif
+       "";
+
+U_BOOT_CMD(mtd, 10, 1, do_mtd, "MTD utils", mtd_help_text);

drivers/mtd/Makefile +1 -1

index cdf515256a..22ceda93c0 100644
@@ -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

drivers/mtd/mtd_uboot.c +160 -1

index 8d7e7890b7..6a211d52ff 100644
@@ -4,8 +4,15 @@
  * Heiko Schocher, DENX Software Engineering, hs at denx.de.
  */
 #include <common.h>
+#include <dm/device.h>
+#include <dm/uclass-internal.h>
+#include <jffs2/jffs2.h> /* LEGACY */
 #include <linux/mtd/mtd.h>
-#include <jffs2/jffs2.h> /* Legacy */
+#include <linux/mtd/partitions.h>
+#include <mtd.h>
+
+#define MTD_NAME_MAX_LEN 20
+
 
 /**
  * mtd_search_alternate_name - Search an alternate name for @mtdname thanks to

@@ -68,6 +75,158 @@ int mtd_search_alternate_name(const char *mtdname, char *altname,
        return -EINVAL;
 }
 
+#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
+
+#if defined(CONFIG_MTD_PARTITIONS)
+int mtd_probe_devices(void)
+{
+       static char *old_mtdparts;
+       static char *old_mtdids;
+       const char *mtdparts = env_get("mtdparts");
+       const char *mtdids = env_get("mtdids");
+       bool remaining_partitions = true;
+       struct mtd_info *mtd;
+
+       mtd_probe_uclass_mtd_devs();
+
+       /* Check if mtdparts/mtdids changed since last call, otherwise: exit */
+       if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
+               return 0;
+
+       /* Update the local copy of mtdparts */
+       free(old_mtdparts);
+       free(old_mtdids);
+       old_mtdparts = strdup(mtdparts);
+       old_mtdids = strdup(mtdids);
+
+       /* 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 -EACCES;
+               }
+       }
+
+       /*
+        * 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 mtd_name[MTD_NAME_MAX_LEN], *colon;
+               struct mtd_partition *parts;
+               int mtd_name_len, nparts;
+               int ret;
+
+               colon = strchr(mtdparts, ':');
+               if (!colon) {
+                       printf("Wrong mtdparts: %s\n", mtdparts);
+                       return -EINVAL;
+               }
+
+               mtd_name_len = colon - mtdparts;
+               strncpy(mtd_name, mtdparts, mtd_name_len);
+               mtd_name[mtd_name_len] = '\0';
+               /* Move the pointer forward (including the ':') */
+               mtdparts += mtd_name_len + 1;
+               mtd = get_mtd_device_nm(mtd_name);
+               if (IS_ERR_OR_NULL(mtd)) {
+                       char linux_name[MTD_NAME_MAX_LEN];
+
+                       /*
+                        * The MTD device named "mtd_name" does not exist. Try
+                        * to find a correspondance with an MTD device having
+                        * the same type and number as defined in the mtdids.
+                        */
+                       debug("No device named %s\n", mtd_name);
+                       ret = mtd_search_alternate_name(mtd_name, linux_name,
+                                                       MTD_NAME_MAX_LEN);
+                       if (!ret)
+                               mtd = get_mtd_device_nm(linux_name);
+
+                       /*
+                        * If no device could be found, move the mtdparts
+                        * pointer forward until the next set of partitions.
+                        */
+                       if (ret || IS_ERR_OR_NULL(mtd)) {
+                               printf("Could not find a valid device for %s\n",
+                                      mtd_name);
+                               mtdparts = strchr(mtdparts, ';');
+                               if (mtdparts)
+                                       mtdparts++;
+
+                               continue;
+                       }
+               }
+
+               /*
+                * 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, &nparts);
+               if (ret) {
+                       printf("Could not parse device %s\n", mtd->name);
+                       put_mtd_device(mtd);
+                       return -EINVAL;
+               }
+
+               if (!nparts)
+                       continue;
+
+               /* Create the new MTD partitions */
+               add_mtd_partitions(mtd, parts, nparts);
+
+               /* Free the structures allocated during the parsing */
+               mtd_free_parsed_partitions(parts, nparts);
+
+               put_mtd_device(mtd);
+       }
+
+       return 0;
+}
+#else
+int mtd_probe_devices(void)
+{
+       mtd_probe_uclass_mtd_devs();
+
+       return 0;
+}
+#endif /* defined(CONFIG_MTD_PARTITIONS) */
+
 /* Legacy */
 
 static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size,

include/mtd.h +16 -1

index 6e6da3002f..011f26b3e1 100644
@@ -8,6 +8,9 @@
 
 #include <linux/mtd/mtd.h>
 
+struct udevice;
+
+#if defined(CONFIG_DM)
 /*
  * Get mtd_info structure of the dev, which is stored as uclass private.
  *

@@ -20,5 +23,18 @@ static inline struct mtd_info *mtd_get_info(struct udevice *dev)
 }
 
 int mtd_probe(struct udevice *dev);
+#else
+static inline struct mtd_info *mtd_get_info(struct udevice *dev)
+{
+       return NULL;
+}
+
+static inline int mtd_probe(struct udevice *dev)
+{
+       return 0;
+}
+#endif
+
+int mtd_probe_devices(void);
 
 #endif /* _MTD_H_ */
--

2.17.1

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Quoted 5 lines

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.

What is the expected behavior when I type 'mtd list' on my omap37 board, it just hangs.

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

I intentionally removed it from the device tree a while ago, because U-Boot was passing the partition info to Linux.

Hi Adam,

Adam Ford <[email protected]> wrote on Wed, 3 Oct 2018 07:35:15 -0500:

Quoted 7 lines · Miquel Raynal

Quoted 5 lines

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.

Quoted 2 lines

What is the expected behavior when I type 'mtd list' on my omap37 board, it just hangs.

What do you mean "hangs", does U-Boot crashes? Or is it really hanging with no more on the console? Can you Ctrl-C to cancel the command or is it really stuck?

Quoted 6 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

Quoted 4 lines

I intentionally removed it from the device tree a while ago, because U-Boot was passing the partition info to Linux.

Indeed, that's his primary role.

Thanks,
Miquèl

Quoted 12 lines

Hi Adam,

Adam Ford <[email protected]> wrote on Wed, 3 Oct 2018 07:35:15 -0500:

Quoted 7 lines · Miquel Raynal

Quoted 5 lines

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.

Quoted 6 lines

Quoted 2 lines

What is the expected behavior when I type 'mtd list' on my omap37 board, it just hangs.

What do you mean "hangs", does U-Boot crashes? Or is it really hanging with no more on the console? Can you Ctrl-C to cancel the command or is it really stuck?

It's really stuck

U-Boot 2018.11-rc1-00636-g592cd5defd (Oct 03 2018 - 07:28:27 -0500)

OMAP3630/3730-GP ES1.2, CPU-OPP2, L3-200MHz, Max CPU Clock 800 MHz
Model: LogicPD Zoom OMAP3 Development Kit
Logic DM37x/OMAP35x reference board + LPDDR/NAND

  DRAM:  256 MiB
  NAND:  512 MiB
  MMC:   OMAP SD/MMC: 0

Loading Environment from NAND... OK
OMAP die ID: 155000029ff800000168301018021018
Board: DM37xx Torpedo
Net: smc911x-0
Hit any key to stop autoboot: 0
OMAP Logic # mtd list

Control-C does nothing.

Quoted 7 lines

Quoted 6 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

Quoted 2 lines

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS

Quoted 7 lines

Quoted 5 lines

I intentionally removed it from the device tree a while ago, because U-Boot was passing the partition info to Linux.

Indeed, that's his primary role.

OK, I just want to make sure I'm understanding it correctly.

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

Quoted 2 lines

OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS

As of today, the process of migration is not entirely finished to DM and you might still need to issue *first* a "nand probe" to register the device operations.

Hi Adam,

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS

Quoted 3 lines

As of today, the process of migration is not entirely finished to DM and you might still need to issue *first* a "nand probe" to register the device operations.

Mmmh there is no nand probe actually, for raw nands like the one you have it should work out of the box.

Quoted 17 lines · Miquel Raynal

Hi Adam,

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS

Quoted 6 lines

Quoted 3 lines

As of today, the process of migration is not entirely finished to DM and you might still need to issue *first* a "nand probe" to register the device operations.

Mmmh there is no nand probe actually, for raw nands like the one you have it should work out of the box.

I have a few tasks to do today for work, but I'll try to do some testing as you suggested this week and possibly later tonight.

adam

Quoted 1 line

Quoted 19 lines · Adam Ford

Quoted 17 lines · Miquel Raynal

Hi Adam,

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

As of today, the process of migration is not entirely finished to DM and you might still need to issue *first* a "nand probe" to register the device operations.

Mmmh there is no nand probe actually, for raw nands like the one you have it should work out of the box.

i haven't actually insterted debug code yet, but I started a quick code review. There is a function called 'mtd_probe_uclass_mtd_devs' which states it will probe with DM compliant drivers.
I am thinking the nand and/or GPMC drivers are not yet DM compliant yet. The DM tree doesn't list any of the MTD parts.

Is it save to assume it just wont' work until the drives are DM compliant, or is this driver designed to play with the lower-level drivers as-is?

Hi Adam,

Quoted 33 lines · On Mon, 8 Oct 2018 11:13:40 -0500 Adam Ford <[email protected]> wrote:

Quoted 19 lines · Adam Ford

Quoted 17 lines · Miquel Raynal

Hi Adam,

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

As of today, the process of migration is not entirely finished to DM and you might still need to issue *first* a "nand probe" to register the device operations.

Mmmh there is no nand probe actually, for raw nands like the one you have it should work out of the box.

i haven't actually insterted debug code yet, but I started a quick code review. There is a function called 'mtd_probe_uclass_mtd_devs' which states it will probe with DM compliant drivers.
I am thinking the nand and/or GPMC drivers are not yet DM compliant yet. The DM tree doesn't list any of the MTD parts.

Quoted 3 lines

Is it save to assume it just wont' work until the drives are DM compliant, or is this driver designed to play with the lower-level drivers as-is?

The whole point of this rework was to allow both old (non-DM compliant) and new (DM compliant) drivers to be exposed the same way to the upper layers. If it doesn't work for regular NAND drivers it's a bug, and we should fix it.

Regards,

Boris

Quoted 39 lines · Boris Brezillon

Hi Adam,

Quoted 33 lines · On Mon, 8 Oct 2018 11:13:40 -0500 Adam Ford <[email protected]> wrote:

Quoted 19 lines · Adam Ford

Quoted 17 lines · Miquel Raynal

Hi Adam,

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

As of today, the process of migration is not entirely finished to DM and you might still need to issue *first* a "nand probe" to register the device operations.

Mmmh there is no nand probe actually, for raw nands like the one you have it should work out of the box.

i haven't actually insterted debug code yet, but I started a quick code review. There is a function called 'mtd_probe_uclass_mtd_devs' which states it will probe with DM compliant drivers.
I am thinking the nand and/or GPMC drivers are not yet DM compliant yet. The DM tree doesn't list any of the MTD parts.

Quoted 8 lines

Quoted 3 lines

Is it save to assume it just wont' work until the drives are DM compliant, or is this driver designed to play with the lower-level drivers as-is?

The whole point of this rework was to allow both old (non-DM compliant) and new (DM compliant) drivers to be exposed the same way to the upper layers. If it doesn't work for regular NAND drivers it's a bug, and we should fix it.

That's what I assummed, so I kept trying to debug, but I wanted to make sure I asked the question, because I've been burned by assumptions before.

I narrowed the hang down to the following in driver/mtd/mtd_uboot, mtd_probe_devices

int mtd_probe_devices(void)
{
...

     /* Check if mtdparts/mtdids changed since last call, otherwise: exit */
     if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
          return 0;
}

The above function neither returns nor exits. I added a brace and a printf just before the return, and the printf never appears. Only when I put #if 0 around the 'if' statement above, and 'mtd list' returned and did a dump.

I tried initializing
static char *old_mtdparts = '\0';
static char *old_mtdids = '\0';

and

static char *old_mtdparts = NULL;
static char *old_mtdids = NULL;

Neither fixed the hang.

adam

The functioning dump (with the ifdef) is as follows:

List of MTD devices:
* nand0
- type: NAND flash
- block size: 0x20000 bytes
- min I/O: 0x800 bytes
- OOB size: 64 bytes
- OOB available: 10 bytes
- ECC strength: 8 bits
- ECC step size: 512 bytes

- bitflip threshold: 6 bits
- 0x000000000000-0x000020000000 : "nand0"
- 0x000000000000-0x000000080000 : "MLO"
- 0x000000080000-0x000000240000 : "u-boot"
- 0x000000240000-0x000000260000 : "spl-os"
- 0x000000260000-0x000000280000 : "u-boot-env"
- 0x000000280000-0x000000880000 : "kernel"
- 0x000000880000-0x000020000000 : "fs"

Quoted 1 line

Quoted 3 lines

Regards,

Boris

Quoted 55 lines · Adam Ford

Quoted 39 lines · Boris Brezillon

Hi Adam,

Quoted 33 lines · On Mon, 8 Oct 2018 11:13:40 -0500 Adam Ford <[email protected]> wrote:

Quoted 19 lines · Adam Ford

Quoted 17 lines · Miquel Raynal

Hi Adam,

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

I can use the nand read/write functions and mtdparts lists the partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where I do store the partition information?

You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions.

OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

As of today, the process of migration is not entirely finished to DM and you might still need to issue *first* a "nand probe" to register the device operations.

Mmmh there is no nand probe actually, for raw nands like the one you have it should work out of the box.

i haven't actually insterted debug code yet, but I started a quick code review. There is a function called 'mtd_probe_uclass_mtd_devs' which states it will probe with DM compliant drivers.
I am thinking the nand and/or GPMC drivers are not yet DM compliant yet. The DM tree doesn't list any of the MTD parts.

Quoted 8 lines

Quoted 3 lines

Is it save to assume it just wont' work until the drives are DM compliant, or is this driver designed to play with the lower-level drivers as-is?

The whole point of this rework was to allow both old (non-DM compliant) and new (DM compliant) drivers to be exposed the same way to the upper layers. If it doesn't work for regular NAND drivers it's a bug, and we should fix it.

That's what I assummed, so I kept trying to debug, but I wanted to make sure I asked the question, because I've been burned by assumptions before.

Quoted 8 lines

I narrowed the hang down to the following in driver/mtd/mtd_uboot, mtd_probe_devices

int mtd_probe_devices(void)
{
...

     /* Check if mtdparts/mtdids changed since last call, otherwise: exit */
     if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))

Quoted 32 lines

          return 0;
}

The above function neither returns nor exits. I added a brace and a printf just before the return, and the printf never appears. Only when I put #if 0 around the 'if' statement above, and 'mtd list' returned and did a dump.

I tried initializing
static char *old_mtdparts = '\0';
static char *old_mtdids = '\0';

and

static char *old_mtdparts = NULL;
static char *old_mtdids = NULL;

Neither fixed the hang.

adam

The functioning dump (with the ifdef) is as follows:

List of MTD devices:
* nand0
- type: NAND flash
- block size: 0x20000 bytes
- min I/O: 0x800 bytes
- OOB size: 64 bytes
- OOB available: 10 bytes
- ECC strength: 8 bits
- ECC step size: 512 bytes

Quoted 12 lines

- bitflip threshold: 6 bits
- 0x000000000000-0x000020000000 : "nand0"
- 0x000000000000-0x000000080000 : "MLO"
- 0x000000080000-0x000000240000 : "u-boot"
- 0x000000240000-0x000000260000 : "spl-os"
- 0x000000260000-0x000000280000 : "u-boot-env"
- 0x000000280000-0x000000880000 : "kernel"
- 0x000000880000-0x000020000000 : "fs"

Quoted 1 line

Lastly, I noticed that I am not able to de-select CMD_MTDPARTS from my config because CMD_UBI (which I use) automatically selects CMD_MTDPARTS. I only mention it because I know you're trying to deprecate CMD_MTDPARTS.

adam

Quoted 3 lines

Quoted 3 lines

Regards,

Boris

drivers/mtd/mtd_uboot.c +5 -0

+#if defined(CONFIG_MTD_PARTITIONS)
+int mtd_probe_devices(void)
+{
+	static char *old_mtdparts;
+	static char *old_mtdids;

drivers/mtd/mtd_uboot.c +24 -0

+
+	/* Update the local copy of mtdparts */
+	free(old_mtdparts);
+	free(old_mtdids);
+	old_mtdparts = strdup(mtdparts);
+	old_mtdids = strdup(mtdids);
+
+	/* 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 -EACCES;
+		}
+	}
+
+	/*
+	 * 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) {

drivers/mtd/mtd_uboot.c +78 -0

+	/* 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 mtd_name[MTD_NAME_MAX_LEN], *colon;
+		struct mtd_partition *parts;
+		int mtd_name_len, nparts;
+		int ret;
+
+		colon = strchr(mtdparts, ':');
+		if (!colon) {
+			printf("Wrong mtdparts: %s\n", mtdparts);
+			return -EINVAL;
+		}
+
+		mtd_name_len = colon - mtdparts;
+		strncpy(mtd_name, mtdparts, mtd_name_len);
+		mtd_name[mtd_name_len] = '\0';
+		/* Move the pointer forward (including the ':') */
+		mtdparts += mtd_name_len + 1;
+		mtd = get_mtd_device_nm(mtd_name);
+		if (IS_ERR_OR_NULL(mtd)) {
+			char linux_name[MTD_NAME_MAX_LEN];
+
+			/*
+			 * The MTD device named "mtd_name" does not exist. Try
+			 * to find a correspondance with an MTD device having
+			 * the same type and number as defined in the mtdids.
+			 */
+			debug("No device named %s\n", mtd_name);
+			ret = mtd_search_alternate_name(mtd_name, linux_name,
+							MTD_NAME_MAX_LEN);
+			if (!ret)
+				mtd = get_mtd_device_nm(linux_name);
+
+			/*
+			 * If no device could be found, move the mtdparts
+			 * pointer forward until the next set of partitions.
+			 */
+			if (ret || IS_ERR_OR_NULL(mtd)) {
+				printf("Could not find a valid device for %s\n",
+				       mtd_name);
+				mtdparts = strchr(mtdparts, ';');
+				if (mtdparts)
+					mtdparts++;
+
+				continue;
+			}
+		}
+
+		/*
+		 * 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, &nparts);
+		if (ret) {
+			printf("Could not parse device %s\n", mtd->name);
+			put_mtd_device(mtd);
+			return -EINVAL;
+		}
+
+		if (!nparts)
+			continue;
+
+		/* Create the new MTD partitions */
+		add_mtd_partitions(mtd, parts, nparts);
+
+		/* Free the structures allocated during the parsing */
+		mtd_free_parsed_partitions(parts, nparts);
+
+		put_mtd_device(mtd);
+	}
+
+	return 0;
+}

Quoted 5 lines · On Mon, 1 Oct 2018 15:43:29 +0200 Miquel Raynal <[email protected]>…

drivers/mtd/mtd_uboot.c +5 -0

+#if defined(CONFIG_MTD_PARTITIONS)
+int mtd_probe_devices(void)
+{
+     static char *old_mtdparts;
+     static char *old_mtdids;

drivers/mtd/mtd_uboot.c int mtd_search_alternate_name(const char *mtdname, char *altname, · line 108

Quoted 10 lines

drivers/mtd/mtd_uboot.c +10 -0

+     const char *mtdparts = env_get("mtdparts");
+     const char *mtdids = env_get("mtdids");
+     bool remaining_partitions = true;
+     struct mtd_info *mtd;
+
+     mtd_probe_uclass_mtd_devs();
+
+     /* Check if mtdparts/mtdids changed since last call, otherwise: exit */
+     if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
+             return 0;

Should be:

        if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) ||
            (mtdparts && old_mtdparts && mtdids && old_mtdids &&
             !strcmp(mtdparts, old_mtdparts) &&
             !strcmp(mtdids, old_mtdids)))
                return 0;

Quoted 24 lines

drivers/mtd/mtd_uboot.c +24 -0

+
+     /* Update the local copy of mtdparts */
+     free(old_mtdparts);
+     free(old_mtdids);
+     old_mtdparts = strdup(mtdparts);
+     old_mtdids = strdup(mtdids);
+
+     /* 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 -EACCES;
+             }
+     }
+
+     /*
+      * 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) {

drivers/mtd/mtd_uboot.c int mtd_search_alternate_name(const char *mtdname, char *altname, · line 142

Quoted 10 lines

drivers/mtd/mtd_uboot.c +10 -0

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

We should bail out if either mtdparts or mtdids is NULL:

        if (!mtdparts || !mtdids)
                return 0;

Quoted 69 lines

drivers/mtd/mtd_uboot.c +68 -0

+     /* 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 mtd_name[MTD_NAME_MAX_LEN], *colon;
+             struct mtd_partition *parts;
+             int mtd_name_len, nparts;
+             int ret;
+
+             colon = strchr(mtdparts, ':');
+             if (!colon) {
+                     printf("Wrong mtdparts: %s\n", mtdparts);
+                     return -EINVAL;
+             }
+
+             mtd_name_len = colon - mtdparts;
+             strncpy(mtd_name, mtdparts, mtd_name_len);
+             mtd_name[mtd_name_len] = '\0';
+             /* Move the pointer forward (including the ':') */
+             mtdparts += mtd_name_len + 1;
+             mtd = get_mtd_device_nm(mtd_name);
+             if (IS_ERR_OR_NULL(mtd)) {
+                     char linux_name[MTD_NAME_MAX_LEN];
+
+                     /*
+                      * The MTD device named "mtd_name" does not exist. Try
+                      * to find a correspondance with an MTD device having
+                      * the same type and number as defined in the mtdids.
+                      */
+                     debug("No device named %s\n", mtd_name);
+                     ret = mtd_search_alternate_name(mtd_name, linux_name,
+                                                     MTD_NAME_MAX_LEN);
+                     if (!ret)
+                             mtd = get_mtd_device_nm(linux_name);
+
+                     /*
+                      * If no device could be found, move the mtdparts
+                      * pointer forward until the next set of partitions.
+                      */
+                     if (ret || IS_ERR_OR_NULL(mtd)) {
+                             printf("Could not find a valid device for %s\n",
+                                    mtd_name);
+                             mtdparts = strchr(mtdparts, ';');
+                             if (mtdparts)
+                                     mtdparts++;
+
+                             continue;
+                     }
+             }
+
+             /*
+              * 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, &nparts);
+             if (ret) {
+                     printf("Could not parse device %s\n", mtd->name);
+                     put_mtd_device(mtd);
+                     return -EINVAL;
+             }
+
+             if (!nparts)
+                     continue;
+
+             /* Create the new MTD partitions */

drivers/mtd/mtd_uboot.c int mtd_search_alternate_name(const char *mtdname, char *altname, · line 220

Quoted 10 lines

drivers/mtd/mtd_uboot.c +10 -0

+             add_mtd_partitions(mtd, parts, nparts);
+
+             /* Free the structures allocated during the parsing */
+             mtd_free_parsed_partitions(parts, nparts);
+
+             put_mtd_device(mtd);
+     }
+
+     return 0;
+}

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

drivers/mtd/mtd_uboot.c int mtd_search_alternate_name(const char *mtdname, char *altname, · line 108

Quoted 3 lines

drivers/mtd/mtd_uboot.c +3 -0

+	/* Check if mtdparts/mtdids changed since last call, otherwise: exit */
+	if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
+		return 0;  

Should be:

	if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) ||

Hello,

Quoted 7 lines · Boris Brezillon

drivers/mtd/mtd_uboot.c int mtd_search_alternate_name(const char *mtdname, char *altname, · line 108

Quoted 3 lines

drivers/mtd/mtd_uboot.c +3 -0

+   /* Check if mtdparts/mtdids changed since last call, otherwise: exit */
+   if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
+           return 0;

Should be:

      if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) ||

If there's a || here, it means you can split the conditions into to:

        if (!mtdparts && !old_mtdparts && !mtdids && !old_mtdids)
                return 0;

        if (mtdparts && old_mtdparts && mtdids && old_mtdids &&
            !strcmp(mtdparts, old_mtdparts) &&
            !strcmp(mtdids, old_mtdids)))
                return 0;

Best regards,

Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot