Re: [U-Boot] [PATCH v6 29/31] cmd: mtd: add 'mtd' command

On Thu, 16 Aug 2018 17:30:27 +0200 Miquel Raynal miquel.raynal@bootlin.com 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 | 7 + cmd/Makefile | 1 + cmd/mtd.c | 513 ++++++++++++++++++++++++++++++++++++++++ drivers/mtd/Makefile | 2 +- include/linux/mtd/mtd.h | 1 + 5 files changed, 523 insertions(+), 1 deletion(-) create mode 100644 cmd/mtd.c

diff --git a/cmd/Kconfig b/cmd/Kconfig index ef43ed8dda..81351920ab 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -847,6 +847,13 @@ config CMD_MMC_SWRITE Enable support for the "mmc swrite" command to write Android sparse images to eMMC.

+config CMD_MTD

• bool "mtd"
• depends on CMD_MTDPARTS
• select MTD_PARTITIONS
• help

•  MTD commands support.
  

config CMD_NAND bool "nand" default y if NAND_SUNXI 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..b9d58725f0 --- /dev/null +++ b/cmd/mtd.c @@ -0,0 +1,513 @@ +// 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 <dm/device.h> +#include <dm/uclass-internal.h>

+#define MTD_NAME_MAX_LEN 20

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

+}

+/*

• • Ensure the MTD device partition list has been initialized already (needed for
• • non DM-compliant devices).
• */

+static void mtd_quirk_init_non_dm_device_lists(void) +{

• struct mtd_info *mtd;
• mtd_for_each_device(mtd) {

• if (!mtd_is_partition(mtd) && !mtd->partitions.next) {
  

• 	INIT_LIST_HEAD(&mtd->partitions);
  

• 	INIT_LIST_HEAD(&mtd->node);
  

• }
  

• }

+}

+#if IS_ENABLED(CONFIG_DM) +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");
• mtd_quirk_init_non_dm_device_lists();
• mtd_probe_uclass_mtd_devs();
• /* Create the partitions defined in mtdparts, here the fun begins */
• /* Ignore the extra 'mtdparts=' prefix if any */
• if (strstr(mtdparts, "mtdparts="))

• mtdparts += 9;
  

• /* For each MTD device in mtdparts */
• while (mtdparts[0] != '\0') {

Hm, so you're unconditionally re-parsing mtdparts, even if nothing changed since the last time you parsed it? Might be expensing if mtd read/write is called iteratively on small MTD portions.

• struct mtd_partition *parts;
  

• struct list_head *iterator;
  

• bool part_in_use = false;
  

• struct mtd_info *parent;
  

• char mtdname[MTD_NAME_MAX_LEN];
  

• int mtdname_len, len, ret;
  

• mtdname_len = strchr(mtdparts, ':') - mtdparts;
  

What if mtdparts is malformed? strchr() might return NULL if it didn't find ':', and you probably want to stop parsing mtdparts in that case.

• strncpy(mtdname, mtdparts, mtdname_len);
  

• mtdname[mtdname_len] = '\0';
  

• /* Move the pointer forward (including the ':') */
  

• mtdparts += mtdname_len + 1;
  

• parent = get_mtd_device_nm(mtdname);
  

• if (IS_ERR_OR_NULL(parent)) {
  

• 	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 = mtdparts_init();
  

• 	if (ret) {
  

• 		printf("Could not initialize mtdparts\n");
  

• 		return ret;
  

• 	}
  

• 	ret = mtdids_search_alternate_name(mtdname, altname,
  

• 					   MTD_NAME_MAX_LEN);
  

• 	if (ret)  {
  

• 		printf("Did not found an equivalent in mtdids\n");
  

• 		return ret;
  

• 	}
  

• 	parent = get_mtd_device_nm(altname);
  

• 	if (IS_ERR_OR_NULL(parent)) {
  

• 		printf("No device named %s\n", altname);
  

• 		return -EINVAL;
  

• 	}
  

• }
  

• put_mtd_device(parent);
  

• /*
  

•  * 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 = mtdparts_parse_part(parent, &mtdparts, &parts, &len);
  

• if (ret) {
  

• 	printf("Could not parse device %s\n", parent->name);
  

• 	return -EINVAL;
  

• }
  

• if (!len)
  

• 	continue;
  

• /* Check that no partition is actually in use */
  

• list_for_each(iterator, &parent->partitions) {
  

• 	struct mtd_info *p = mtd_part_node_to_info(iterator);
  

• 	if (p->usecount) {
  

• 		printf("MTD partition \"%s\" already in use.\n",
  

• 		       p->name);
  

• 		part_in_use = true;
  

• 	}
  

• }
  

• if (part_in_use) {
  

• 	printf("Not reconstructing partition list for MTD device %s\n",
  

• 	       parent->name);
  

• 	continue;
  

• }
  

• /* Free the existing partitions */
  

• del_mtd_partitions(parent);
  

• /* Create the new MTD partitions */
  

• add_mtd_partitions(parent, parts, len);
  

• free(parts);
  

• }
• return 0;

+}

+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;
• /* 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);
  

I would print partitions too, and if possible, put them under the master device with an extra level of indentation.

• dev_nb++;
  

• }
• if (!dev_nb) {

• printf("No MTD device found\n");
  

• return CMD_RET_FAILURE;
  

• }
• return 0;

+}

+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 = get_mtd_device_nm(mtdname);
• if (IS_ERR_OR_NULL(mtd)) {

• mtd_probe_devices();
  

Actually this should be done unconditionally, just in case the object returned by get_mtd_device_nm() is a partition. mtdparts env var might have changed between the 2 mtd xxx calls, and the partition we've been given might no longer be valid. The solution would be to avoid re-parsing things when mtdparts didn't change (you can check that in 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 1;
  

return CMD_RET_FAILURE;

• }
  

• }
• put_mtd_device(mtd);

Why do you call put_mtd_device() here? You should retain the reference until you're done interacting with the device.