[PATCH v5 0/4] MTD concat

[PATCH v5 0/4] MTD concat

[Miquel Raynal]

Hello,

A year ago Bernhard Frauendienst started an effort to bring MTD
devices concatenation generic [1]. Today I also need this
concatenation to be possible in order to support configurations where
two MTD devices are treated like one bigger in order to be able to
define partitions across chip boundaries.

After having talked with Mark Brown, Boris Brezillon and Rob Herring,
the only approach which seems acceptable is to add a property in the
partitions nodes to describe which partitions should be concatenated
in a virtual device.

At first I changed a bit the code logic and style, keeping the logic
from the original version. Since the last bindings change, I rewrote
almost all the driver, so I took ownership on it, keeping Bernhard in
a 'Suggested-by' tag.

I would like to add another way to concatenate devices: with module
parameters/arguments on the cmdline. This is easily doable in a second
time.

Thanks,
Miquèl

[1] https://lwn.net/ml/linux-kernel/20180907173515.19990-1-kernel@nospam.obeliks.de/


Bernhard Frauendienst (1):
  mtd: Add get_mtd_device_by_node() helper

Miquel Raynal (3):
  dt-bindings: mtd: Describe MTD partitions concatenation
  mtd: concat: Fix a comment referring to an unknown symbol
  mtd: Add driver for concatenating devices

 .../devicetree/bindings/mtd/partition.txt     |   1 +
 drivers/mtd/Kconfig                           |   8 +
 drivers/mtd/Makefile                          |   1 +
 drivers/mtd/mtd_virt_concat.c                 | 240 ++++++++++++++++++
 drivers/mtd/mtdconcat.c                       |   5 +-
 drivers/mtd/mtdcore.c                         |  38 +++
 include/linux/mtd/mtd.h                       |   2 +
 7 files changed, 291 insertions(+), 4 deletions(-)
 create mode 100644 drivers/mtd/mtd_virt_concat.c

-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 1/4] dt-bindings: mtd: Describe MTD partitions concatenation

[Miquel Raynal]

The main use case to concatenate MTD devices is probably SPI-NOR
flashes where the number of address bits is limited to 24, which can
access a range of 16MiB. Board manufacturers might want to double the
SPI storage size by adding a second flash asserted thanks to a second
chip selects which enhances the addressing capabilities to 25 bits,
32MiB. Having two devices for twice the size is great but without more
glue, we cannot define partition boundaries spread across the two
devices. This is the gap mtd-concat intends to address.

There are three options to describe concatenated devices:
1/ One flash chip is described in the DT with two CS;
2/ Two flash chips are described in the DT with one CS each, a virtual
   device is also created to describe the concatenation.
3/ Partitions that must be concatenated are described in the
   partitions subnodes in a specific part-concat property.

Solution 1/ presents at least 3 issues:
* The hardware description is abused;
* The concatenation only works for SPI devices (while it could be
  helpful for any MTD);
* It would require a lot of rework in the SPI core as most of the
  logic assumes there is and there always will be only one CS per
  chip.

Solution 2/ also has caveats:
* The virtual device has no hardware reality;
* Possible optimizations at the hardware level will be hard to enable
  efficiently (ie. a common direct mapping abstracted by a SPI
  memories oriented controller).

Solution 3/ is maybe better from the bindings point of view but
introduces a real mess in kernel code and the amount of boilerplate is
insane compared to solution 2. This is the one finally implemented.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 Documentation/devicetree/bindings/mtd/partition.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
index afbbd870496d..6e3ac87ff988 100644
--- a/Documentation/devicetree/bindings/mtd/partition.txt
+++ b/Documentation/devicetree/bindings/mtd/partition.txt
@@ -61,6 +61,7 @@ Optional properties:
   clobbered.
 - lock : Do not unlock the partition at initialization time (not supported on
   all devices)
+- part-concat : List of MTD partitions phandles that should be concatenated.
 
 Examples:
 
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 2/4] mtd: concat: Fix a comment referring to an unknown symbol

[Miquel Raynal]

Fix the comment describing what the mtd_concat_destroy() function
does. It referrers to the concat_mtd_devs symbol which has never
existed (at least not since the beginning of the Git era).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/mtdconcat.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index cbc5925e6440..1b6428d6e13d 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -852,10 +852,7 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[],	/* subdevices to c
 	return &concat->mtd;
 }
 
-/*
- * This function destroys an MTD object obtained from concat_mtd_devs()
- */
-
+/* Cleans the context obtained from mtd_concat_create() */
 void mtd_concat_destroy(struct mtd_info *mtd)
 {
 	struct mtd_concat *concat = CONCAT(mtd);
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 3/4] mtd: Add get_mtd_device_by_node() helper

[Miquel Raynal]

From: Bernhard Frauendienst <kernel@nospam.obeliks.de>

Add an helper to retrieve a MTD device by its OF node. Since drivers can
assign arbitrary names to MTD devices in the absence of a 'label' DT
property, there is no other reliable way to retrieve a MTD device for a
given OF node.

Signed-off-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
[<miquel.raynal@bootlin.com>: light internals rework]
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/mtdcore.c   | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 76b4264936ff..5a94a2c0a6de 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -985,6 +985,44 @@ struct mtd_info *get_mtd_device_nm(const char *name)
 }
 EXPORT_SYMBOL_GPL(get_mtd_device_nm);
 
+/**
+ *	get_mtd_device_by_node - obtain a validated handle for an MTD device
+ *	by of_node
+ *	@of_node: OF node of MTD device to open
+ *
+ *	This function returns an MTD device structure in case of success,
+ *	an error code otherwise.
+ */
+struct mtd_info *get_mtd_device_by_node(const struct device_node *of_node)
+{
+	struct mtd_info *mtd;
+	bool found = false;
+	int ret;
+
+	mutex_lock(&mtd_table_mutex);
+
+	mtd_for_each_device(mtd) {
+		if (of_node == mtd->dev.of_node) {
+			found = true;
+			break;
+		}
+	}
+
+	if (found)
+		ret = __get_mtd_device(mtd);
+
+	mutex_unlock(&mtd_table_mutex);
+
+	if (!found)
+		return ERR_PTR(-ENODEV);
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	return mtd;
+}
+EXPORT_SYMBOL_GPL(get_mtd_device_by_node);
+
 void put_mtd_device(struct mtd_info *mtd)
 {
 	mutex_lock(&mtd_table_mutex);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 677768b21a1d..0f25c476a1a3 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -573,6 +573,8 @@ extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
 extern int __get_mtd_device(struct mtd_info *mtd);
 extern void __put_mtd_device(struct mtd_info *mtd);
 extern struct mtd_info *get_mtd_device_nm(const char *name);
+extern struct mtd_info *get_mtd_device_by_node(
+		const struct device_node *of_node);
 extern void put_mtd_device(struct mtd_info *mtd);
 
 
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 4/4] mtd: Add driver for concatenating devices

[Miquel Raynal]

Introduce a generic way to define concatenated MTD devices. This may
be very useful in the case of ie. stacked SPI-NOR. Partitions to
concatenate are described in an additional property of the partitions
subnode:

        flash0 {
                partitions {
                        compatible = "fixed-partitions";
                        part-concat = <&flash0_part1>, <&flash1_part0>;

			part0@0 {
				label = "part0_0";
				reg = <0x0 0x800000>;
			};

			flash0_part1: part1@800000 {
				label = "part0_1";
				reg = <0x800000 0x800000>;
			};
                };
        };

        flash1 {
                partitions {
                        compatible = "fixed-partitions";

			flash0_part1: part1@0 {
				label = "part1_0";
				reg = <0x0 0x800000>;
			};

			part0@800000 {
				label = "part1_1";
				reg = <0x800000 0x800000>;
			};
                };
        };

This is useful for boards where memory range has been extended with
the use of multiple flash chips as memory banks of a single MTD
device, with partitions spanning chip borders.

Suggested-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/Kconfig           |   8 ++
 drivers/mtd/Makefile          |   1 +
 drivers/mtd/mtd_virt_concat.c | 240 ++++++++++++++++++++++++++++++++++
 3 files changed, 249 insertions(+)
 create mode 100644 drivers/mtd/mtd_virt_concat.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 79a8ff542883..3e1e55e7158f 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -276,6 +276,14 @@ config MTD_PARTITIONED_MASTER
 	  the parent of the partition device be the master device, rather than
 	  what lies behind the master.
 
+config MTD_VIRT_CONCAT
+	tristate "Virtual concatenated MTD devices"
+	help
+	  This driver allows creation of a virtual MTD device, which
+	  concatenates multiple physical MTD devices into a single one.
+	  This is useful to create partitions bigger than the underlying
+	  physical chips by allowing cross-chip boundaries.
+
 source "drivers/mtd/chips/Kconfig"
 
 source "drivers/mtd/maps/Kconfig"
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 58fc327a5276..c7ee13368a66 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_SSFDC)		+= ssfdc.o
 obj-$(CONFIG_SM_FTL)		+= sm_ftl.o
 obj-$(CONFIG_MTD_OOPS)		+= mtdoops.o
 obj-$(CONFIG_MTD_SWAP)		+= mtdswap.o
+obj-$(CONFIG_MTD_VIRT_CONCAT)	+= mtd_virt_concat.o
 
 nftl-objs		:= nftlcore.o nftlmount.o
 inftl-objs		:= inftlcore.o inftlmount.o
diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
new file mode 100644
index 000000000000..23c7170ac32f
--- /dev/null
+++ b/drivers/mtd/mtd_virt_concat.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Virtual concat MTD device driver
+ *
+ * Copyright (C) 2018 Bernhard Frauendienst
+ * Author: Bernhard Frauendienst <kernel@nospam.obeliks.de>
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/mtd/concat.h>
+#include <linux/mtd/mtd.h>
+#include "mtdcore.h"
+#include <linux/mtd/partitions.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+#define CONCAT_PROP "part-concat"
+#define MIN_DEV_PER_CONCAT 2
+
+/**
+ * struct mtd_virt_concat - concatenation container
+ * @vmtd: Virtual mtd_concat device
+ * @count: Number of physical underlaying devices in @devices
+ * @devices: Array of the physical devices used
+ */
+struct mtd_virt_concat {
+	struct mtd_info	*vmtd;
+	unsigned int count;
+	struct mtd_info	**devices;
+};
+
+/**
+ * struct mtd_virt_concat_node - components of a concatenation
+ * @head: List handle
+ * @count: Number of nodes
+ * @nodes: Pointer to the nodes (partitions) to concatenate
+ * @concat: Concatenation container
+ */
+struct mtd_virt_concat_node {
+	struct list_head head;
+	unsigned int count;
+	struct device_node **nodes;
+	struct mtd_virt_concat *concat;
+};
+
+static LIST_HEAD(concat_list);
+
+static void mtd_virt_concat_put_mtd_devices(struct mtd_virt_concat *concat)
+{
+	int i;
+
+	for (i = 0; i < concat->count; i++)
+		put_mtd_device(concat->devices[i]);
+}
+
+static int mtd_virt_concat_create_join(struct mtd_virt_concat_node *item)
+{
+	struct mtd_virt_concat *concat;
+	struct mtd_info *mtd;
+	ssize_t name_sz;
+	char *name;
+	int ret, i;
+
+	concat = kzalloc(sizeof(*concat), GFP_KERNEL);
+	if (!concat)
+		return -ENOMEM;
+
+	concat->devices = kcalloc(item->count,
+				  sizeof(*concat->devices),
+				  GFP_KERNEL);
+	if (!concat->devices) {
+		ret = -ENOMEM;
+		goto free_concat;
+	}
+
+	/* Aggregate the physical devices */
+	for (i = 0; i < item->count; i++) {
+		mtd = get_mtd_device_by_node(item->nodes[i]);
+		if (IS_ERR(mtd)) {
+			ret = PTR_ERR(mtd);
+			goto put_mtd_devices;
+		}
+
+		concat->devices[concat->count++] = mtd;
+	}
+
+	/* Create the virtual device */
+	name_sz = snprintf(NULL, 0, "%s-%s%s-concat",
+			   concat->devices[0]->name,
+			   concat->devices[1]->name,
+			   concat->count > 2 ? "-+" : "");
+	name = kmalloc(name_sz, GFP_KERNEL);
+	if (!name) {
+		ret = -ENOMEM;
+		goto put_mtd_devices;
+	}
+
+	sprintf(name, "%s-%s%s-concat",
+		concat->devices[0]->name,
+		concat->devices[1]->name,
+		concat->count > 2 ? "-+" : "");
+
+	concat->vmtd = mtd_concat_create(concat->devices, concat->count, name);
+	if (!concat->vmtd) {
+		ret = -ENXIO;
+		goto free_name;
+	}
+
+	/* Arbitrary set the first device as parent */
+	concat->vmtd->dev.parent = &concat->devices[0]->dev;
+
+	/* Register the platform device */
+	ret = mtd_device_register(concat->vmtd, NULL, 0);
+	if (ret)
+		goto destroy_concat;
+
+	item->concat = concat;
+
+	return 0;
+
+destroy_concat:
+	mtd_concat_destroy(concat->vmtd);
+free_name:
+	kfree(name);
+put_mtd_devices:
+	mtd_virt_concat_put_mtd_devices(concat);
+free_concat:
+	kfree(concat);
+
+	return ret;
+}
+
+static void mtd_virt_concat_destroy_joins(void)
+{
+	struct mtd_virt_concat_node *item, *tmp;
+
+	list_for_each_entry_safe(item, tmp, &concat_list, head) {
+		if (item->concat) {
+			mtd_device_unregister(item->concat->vmtd);
+			kfree(item->concat->vmtd->name);
+			mtd_concat_destroy(item->concat->vmtd);
+			mtd_virt_concat_put_mtd_devices(item->concat);
+		}
+	}
+}
+
+static int mtd_virt_concat_create_item(struct device_node *parts,
+				       unsigned int count)
+{
+	struct mtd_virt_concat_node *item;
+	int i;
+
+	item = kzalloc(sizeof(*item), GFP_KERNEL);
+	if (!item)
+		return -ENOMEM;
+
+	item->count = count;
+	item->nodes = kcalloc(count, sizeof(*item->nodes), GFP_KERNEL);
+	if (!item->nodes) {
+		kfree(item);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < count; i++)
+		item->nodes[i] = of_parse_phandle(parts, CONCAT_PROP, i);
+
+	list_add_tail(&item->head, &concat_list);
+
+	return 0;
+}
+
+static void mtd_virt_concat_destroy_items(void)
+{
+	struct mtd_virt_concat_node *item, *temp;
+	int i;
+
+	list_for_each_entry_safe(item, temp, &concat_list, head) {
+		for (i = 0; i < item->count; i++)
+			of_node_put(item->nodes[i]);
+
+		kfree(item->nodes);
+		kfree(item);
+	}
+}
+
+static int __init mtd_virt_concat_init(void)
+{
+	struct mtd_virt_concat_node *item;
+	struct device_node *parts = NULL;
+	int ret = 0, count;
+
+	/* List all the concatenations found in DT */
+	do {
+		parts = of_find_node_with_property(parts, CONCAT_PROP);
+		if (!of_device_is_available(parts))
+			continue;
+
+		count = of_count_phandle_with_args(parts, CONCAT_PROP, NULL);
+		if (count < MIN_DEV_PER_CONCAT)
+			continue;
+
+		ret = mtd_virt_concat_create_item(parts, count);
+		if (ret) {
+			of_node_put(parts);
+			goto destroy_items;
+		}
+	} while (parts);
+
+	/* TODO: also parse the cmdline */
+
+	/* Create the concatenations */
+	list_for_each_entry(item, &concat_list, head) {
+		ret = mtd_virt_concat_create_join(item);
+		if (ret)
+			goto destroy_joins;
+	}
+
+	return 0;
+
+destroy_joins:
+	mtd_virt_concat_destroy_joins();
+destroy_items:
+	mtd_virt_concat_destroy_items();
+
+	return ret;
+}
+late_initcall(mtd_virt_concat_init);
+
+static void __exit mtd_virt_concat_exit(void)
+{
+	mtd_virt_concat_destroy_joins();
+	mtd_virt_concat_destroy_items();
+}
+module_exit(mtd_virt_concat_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Bernhard Frauendienst <kernel@nospam.obeliks.de>");
+MODULE_DESCRIPTION("Virtual concat MTD device driver");
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices

[Vignesh Raghavendra]

Hi Miquel,

On 27/11/19 4:25 pm, Miquel Raynal wrote:
> Introduce a generic way to define concatenated MTD devices. This may
> be very useful in the case of ie. stacked SPI-NOR. Partitions to
> concatenate are described in an additional property of the partitions
> subnode:
> 
>         flash0 {
>                 partitions {
>                         compatible = "fixed-partitions";
>                         part-concat = <&flash0_part1>, <&flash1_part0>;
> 
> 			part0@0 {
> 				label = "part0_0";
> 				reg = <0x0 0x800000>;
> 			};
> 
> 			flash0_part1: part1@800000 {
> 				label = "part0_1";
> 				reg = <0x800000 0x800000>;
> 			};
>                 };
>         };

IIUC flash0 and flash1 are subnodes of a SPI master node?
And I believe flash0 node's compatible is "jedec,spi-nor"?


> 
>         flash1 {
>                 partitions {
>                         compatible = "fixed-partitions";
> 
> 			flash0_part1: part1@0 {

s/flash0_part1/flash1_part0?

> 				label = "part1_0";
> 				reg = <0x0 0x800000>;
> 			};
> 
> 			part0@800000 {
> 				label = "part1_1";
> 				reg = <0x800000 0x800000>;
> 			};
>                 };
>         };
> 

For my understanding, how many /dev/mtdX entries would this create?

Regards
Vignesh

> This is useful for boards where memory range has been extended with
> the use of multiple flash chips as memory banks of a single MTD
> device, with partitions spanning chip borders.
> 
> Suggested-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/Kconfig           |   8 ++
>  drivers/mtd/Makefile          |   1 +
>  drivers/mtd/mtd_virt_concat.c | 240 ++++++++++++++++++++++++++++++++++
>  3 files changed, 249 insertions(+)
>  create mode 100644 drivers/mtd/mtd_virt_concat.c
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 79a8ff542883..3e1e55e7158f 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -276,6 +276,14 @@ config MTD_PARTITIONED_MASTER
>  	  the parent of the partition device be the master device, rather than
>  	  what lies behind the master.
>  
> +config MTD_VIRT_CONCAT
> +	tristate "Virtual concatenated MTD devices"
> +	help
> +	  This driver allows creation of a virtual MTD device, which
> +	  concatenates multiple physical MTD devices into a single one.
> +	  This is useful to create partitions bigger than the underlying
> +	  physical chips by allowing cross-chip boundaries.
> +
>  source "drivers/mtd/chips/Kconfig"
>  
>  source "drivers/mtd/maps/Kconfig"
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 58fc327a5276..c7ee13368a66 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_SSFDC)		+= ssfdc.o
>  obj-$(CONFIG_SM_FTL)		+= sm_ftl.o
>  obj-$(CONFIG_MTD_OOPS)		+= mtdoops.o
>  obj-$(CONFIG_MTD_SWAP)		+= mtdswap.o
> +obj-$(CONFIG_MTD_VIRT_CONCAT)	+= mtd_virt_concat.o
>  
>  nftl-objs		:= nftlcore.o nftlmount.o
>  inftl-objs		:= inftlcore.o inftlmount.o
> diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
> new file mode 100644
> index 000000000000..23c7170ac32f
> --- /dev/null
> +++ b/drivers/mtd/mtd_virt_concat.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Virtual concat MTD device driver
> + *
> + * Copyright (C) 2018 Bernhard Frauendienst
> + * Author: Bernhard Frauendienst <kernel@nospam.obeliks.de>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/mtd/concat.h>
> +#include <linux/mtd/mtd.h>
> +#include "mtdcore.h"
> +#include <linux/mtd/partitions.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +
> +#define CONCAT_PROP "part-concat"
> +#define MIN_DEV_PER_CONCAT 2
> +
> +/**
> + * struct mtd_virt_concat - concatenation container
> + * @vmtd: Virtual mtd_concat device
> + * @count: Number of physical underlaying devices in @devices
> + * @devices: Array of the physical devices used
> + */
> +struct mtd_virt_concat {
> +	struct mtd_info	*vmtd;
> +	unsigned int count;
> +	struct mtd_info	**devices;
> +};
> +
> +/**
> + * struct mtd_virt_concat_node - components of a concatenation
> + * @head: List handle
> + * @count: Number of nodes
> + * @nodes: Pointer to the nodes (partitions) to concatenate
> + * @concat: Concatenation container
> + */
> +struct mtd_virt_concat_node {
> +	struct list_head head;
> +	unsigned int count;
> +	struct device_node **nodes;
> +	struct mtd_virt_concat *concat;
> +};
> +
> +static LIST_HEAD(concat_list);
> +
> +static void mtd_virt_concat_put_mtd_devices(struct mtd_virt_concat *concat)
> +{
> +	int i;
> +
> +	for (i = 0; i < concat->count; i++)
> +		put_mtd_device(concat->devices[i]);
> +}
> +
> +static int mtd_virt_concat_create_join(struct mtd_virt_concat_node *item)
> +{
> +	struct mtd_virt_concat *concat;
> +	struct mtd_info *mtd;
> +	ssize_t name_sz;
> +	char *name;
> +	int ret, i;
> +
> +	concat = kzalloc(sizeof(*concat), GFP_KERNEL);
> +	if (!concat)
> +		return -ENOMEM;
> +
> +	concat->devices = kcalloc(item->count,
> +				  sizeof(*concat->devices),
> +				  GFP_KERNEL);
> +	if (!concat->devices) {
> +		ret = -ENOMEM;
> +		goto free_concat;
> +	}
> +
> +	/* Aggregate the physical devices */
> +	for (i = 0; i < item->count; i++) {
> +		mtd = get_mtd_device_by_node(item->nodes[i]);
> +		if (IS_ERR(mtd)) {
> +			ret = PTR_ERR(mtd);
> +			goto put_mtd_devices;
> +		}
> +
> +		concat->devices[concat->count++] = mtd;
> +	}
> +
> +	/* Create the virtual device */
> +	name_sz = snprintf(NULL, 0, "%s-%s%s-concat",
> +			   concat->devices[0]->name,
> +			   concat->devices[1]->name,
> +			   concat->count > 2 ? "-+" : "");
> +	name = kmalloc(name_sz, GFP_KERNEL);
> +	if (!name) {
> +		ret = -ENOMEM;
> +		goto put_mtd_devices;
> +	}
> +
> +	sprintf(name, "%s-%s%s-concat",
> +		concat->devices[0]->name,
> +		concat->devices[1]->name,
> +		concat->count > 2 ? "-+" : "");
> +
> +	concat->vmtd = mtd_concat_create(concat->devices, concat->count, name);
> +	if (!concat->vmtd) {
> +		ret = -ENXIO;
> +		goto free_name;
> +	}
> +
> +	/* Arbitrary set the first device as parent */
> +	concat->vmtd->dev.parent = &concat->devices[0]->dev;
> +
> +	/* Register the platform device */
> +	ret = mtd_device_register(concat->vmtd, NULL, 0);
> +	if (ret)
> +		goto destroy_concat;
> +
> +	item->concat = concat;
> +
> +	return 0;
> +
> +destroy_concat:
> +	mtd_concat_destroy(concat->vmtd);
> +free_name:
> +	kfree(name);
> +put_mtd_devices:
> +	mtd_virt_concat_put_mtd_devices(concat);
> +free_concat:
> +	kfree(concat);
> +
> +	return ret;
> +}
> +
> +static void mtd_virt_concat_destroy_joins(void)
> +{
> +	struct mtd_virt_concat_node *item, *tmp;
> +
> +	list_for_each_entry_safe(item, tmp, &concat_list, head) {
> +		if (item->concat) {
> +			mtd_device_unregister(item->concat->vmtd);
> +			kfree(item->concat->vmtd->name);
> +			mtd_concat_destroy(item->concat->vmtd);
> +			mtd_virt_concat_put_mtd_devices(item->concat);
> +		}
> +	}
> +}
> +
> +static int mtd_virt_concat_create_item(struct device_node *parts,
> +				       unsigned int count)
> +{
> +	struct mtd_virt_concat_node *item;
> +	int i;
> +
> +	item = kzalloc(sizeof(*item), GFP_KERNEL);
> +	if (!item)
> +		return -ENOMEM;
> +
> +	item->count = count;
> +	item->nodes = kcalloc(count, sizeof(*item->nodes), GFP_KERNEL);
> +	if (!item->nodes) {
> +		kfree(item);
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < count; i++)
> +		item->nodes[i] = of_parse_phandle(parts, CONCAT_PROP, i);
> +
> +	list_add_tail(&item->head, &concat_list);
> +
> +	return 0;
> +}
> +
> +static void mtd_virt_concat_destroy_items(void)
> +{
> +	struct mtd_virt_concat_node *item, *temp;
> +	int i;
> +
> +	list_for_each_entry_safe(item, temp, &concat_list, head) {
> +		for (i = 0; i < item->count; i++)
> +			of_node_put(item->nodes[i]);
> +
> +		kfree(item->nodes);
> +		kfree(item);
> +	}
> +}
> +
> +static int __init mtd_virt_concat_init(void)
> +{
> +	struct mtd_virt_concat_node *item;
> +	struct device_node *parts = NULL;
> +	int ret = 0, count;
> +
> +	/* List all the concatenations found in DT */
> +	do {
> +		parts = of_find_node_with_property(parts, CONCAT_PROP);
> +		if (!of_device_is_available(parts))
> +			continue;
> +
> +		count = of_count_phandle_with_args(parts, CONCAT_PROP, NULL);
> +		if (count < MIN_DEV_PER_CONCAT)
> +			continue;
> +
> +		ret = mtd_virt_concat_create_item(parts, count);
> +		if (ret) {
> +			of_node_put(parts);
> +			goto destroy_items;
> +		}
> +	} while (parts);
> +
> +	/* TODO: also parse the cmdline */
> +
> +	/* Create the concatenations */
> +	list_for_each_entry(item, &concat_list, head) {
> +		ret = mtd_virt_concat_create_join(item);
> +		if (ret)
> +			goto destroy_joins;
> +	}
> +
> +	return 0;
> +
> +destroy_joins:
> +	mtd_virt_concat_destroy_joins();
> +destroy_items:
> +	mtd_virt_concat_destroy_items();
> +
> +	return ret;
> +}
> +late_initcall(mtd_virt_concat_init);
> +
> +static void __exit mtd_virt_concat_exit(void)
> +{
> +	mtd_virt_concat_destroy_joins();
> +	mtd_virt_concat_destroy_items();
> +}
> +module_exit(mtd_virt_concat_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Bernhard Frauendienst <kernel@nospam.obeliks.de>");
> +MODULE_DESCRIPTION("Virtual concat MTD device driver");
> 

-- 
Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices

[Miquel Raynal]

Hi Vignesh,

Vignesh Raghavendra <vigneshr@ti.com> wrote on Wed, 4 Dec 2019 15:28:46
+0530:

> Hi Miquel,
> 
> On 27/11/19 4:25 pm, Miquel Raynal wrote:
> > Introduce a generic way to define concatenated MTD devices. This may
> > be very useful in the case of ie. stacked SPI-NOR. Partitions to
> > concatenate are described in an additional property of the partitions
> > subnode:
> > 
> >         flash0 {
> >                 partitions {
> >                         compatible = "fixed-partitions";
> >                         part-concat = <&flash0_part1>, <&flash1_part0>;
> > 
> > 			part0@0 {
> > 				label = "part0_0";
> > 				reg = <0x0 0x800000>;
> > 			};
> > 
> > 			flash0_part1: part1@800000 {
> > 				label = "part0_1";
> > 				reg = <0x800000 0x800000>;
> > 			};
> >                 };
> >         };  
> 
> IIUC flash0 and flash1 are subnodes of a SPI master node?
> And I believe flash0 node's compatible is "jedec,spi-nor"?

Indeed this is one possibility (probably the most common) but in theory
this should work for any kind of MTD device, hence I voluntarily
dropped the hardware-specific properties to focus on the partitions
description here.

> 
> 
> > 
> >         flash1 {
> >                 partitions {
> >                         compatible = "fixed-partitions";
> > 
> > 			flash0_part1: part1@0 {  
> 
> s/flash0_part1/flash1_part0?

Right!

> 
> > 				label = "part1_0";
> > 				reg = <0x0 0x800000>;
> > 			};
> > 
> > 			part0@800000 {
> > 				label = "part1_1";
> > 				reg = <0x800000 0x800000>;
> > 			};
> >                 };
> >         };
> >   
> 
> For my understanding, how many /dev/mtdX entries would this create?

If the master is retained (Kconfig option) and thanks to the common
partitioning scheme, we would have:
* flash0 (mtd0)
*   part0_0 (mtd1)
*   part0_1 (mtd2)
* flash1 (mtd3)
*   part1_0 (mtd4)
*   part1_1 (mtd5)

If we enable this driver, we would also get an additional device:
* mtd2-mtd4-concat (or part0_1-part1_0-concat, I don't recall the exact
  name) being mtd6.


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices

[Vignesh Raghavendra]

On 04/12/19 3:47 pm, Miquel Raynal wrote:
> Hi Vignesh,
> 
> Vignesh Raghavendra <vigneshr@ti.com> wrote on Wed, 4 Dec 2019 15:28:46
> +0530:
> 
[...]
>> IIUC flash0 and flash1 are subnodes of a SPI master node?
>> And I believe flash0 node's compatible is "jedec,spi-nor"?
> 
> Indeed this is one possibility (probably the most common) but in theory
> this should work for any kind of MTD device, hence I voluntarily
> dropped the hardware-specific properties to focus on the partitions
> description here.
> 

Ah, make sense...

>>
>>
>>>
>>>         flash1 {
>>>                 partitions {
>>>                         compatible = "fixed-partitions";
>>>
>>> 			flash0_part1: part1@0 {  
>>
>> s/flash0_part1/flash1_part0?
> 
> Right!
> 
>>
>>> 				label = "part1_0";
>>> 				reg = <0x0 0x800000>;
>>> 			};
>>>
>>> 			part0@800000 {
>>> 				label = "part1_1";
>>> 				reg = <0x800000 0x800000>;
>>> 			};
>>>                 };
>>>         };
>>>   
>>
>> For my understanding, how many /dev/mtdX entries would this create?
> 
> If the master is retained (Kconfig option) and thanks to the common
> partitioning scheme, we would have:
> * flash0 (mtd0)
> *   part0_0 (mtd1)
> *   part0_1 (mtd2)
> * flash1 (mtd3)
> *   part1_0 (mtd4)
> *   part1_1 (mtd5)
> 
> If we enable this driver, we would also get an additional device:
> * mtd2-mtd4-concat (or part0_1-part1_0-concat, I don't recall the exact
>   name) being mtd6.

Ok, thanks for the clarification!


-- 
Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices

[Boris Brezillon]

On Wed, 27 Nov 2019 11:55:22 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Introduce a generic way to define concatenated MTD devices. This may
> be very useful in the case of ie. stacked SPI-NOR. Partitions to
> concatenate are described in an additional property of the partitions
> subnode:
> 
>         flash0 {
>                 partitions {
>                         compatible = "fixed-partitions";
>                         part-concat = <&flash0_part1>, <&flash1_part0>;
> 
> 			part0@0 {
> 				label = "part0_0";
> 				reg = <0x0 0x800000>;
> 			};
> 
> 			flash0_part1: part1@800000 {
> 				label = "part0_1";
> 				reg = <0x800000 0x800000>;

So, flash0_part1 and flash0_part2 will be created even though the user
probably doesn't need them?

> 			};
>                 };
>         };
> 
>         flash1 {
>                 partitions {
>                         compatible = "fixed-partitions";
> 
> 			flash0_part1: part1@0 {
> 				label = "part1_0";
> 				reg = <0x0 0x800000>;
> 			};
> 
> 			part0@800000 {
> 				label = "part1_1";
> 				reg = <0x800000 0x800000>;
> 			};
>                 };
>         };

IMHO this representation is far from intuitive. At first glance it's not
obvious which partitions are linked together and what's the name of the
resulting concatenated part. I definitely prefer the solution where we
have a virtual device describing the concatenation. I also understand
that this goes against the #1 DT rule: "DT only decribes HW blocks, not
how they should be used/configured", but maybe we can find a compromise
here, like moving this description to the /chosen node?

chosen {
	flash-arrays {
		/*
		 * my-flash-array is the MTD name if label is
		 * not present.
		 */
		my-flash-array {
			/*
			 * We could have
			 * compatible = "flash-array";
			 * but we can also do without it.
			 */
			label = "foo";
			flashes = <&flash1 &flash2 ...>;
			partitions {
				/* usual partition description. */
				...
			};
		};
	};
};

Rob, what do you think?

> 
> This is useful for boards where memory range has been extended with
> the use of multiple flash chips as memory banks of a single MTD
> device, with partitions spanning chip borders.
> 
> Suggested-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/Kconfig           |   8 ++
>  drivers/mtd/Makefile          |   1 +
>  drivers/mtd/mtd_virt_concat.c | 240 ++++++++++++++++++++++++++++++++++
>  3 files changed, 249 insertions(+)
>  create mode 100644 drivers/mtd/mtd_virt_concat.c
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 79a8ff542883..3e1e55e7158f 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -276,6 +276,14 @@ config MTD_PARTITIONED_MASTER
>  	  the parent of the partition device be the master device, rather than
>  	  what lies behind the master.
>  
> +config MTD_VIRT_CONCAT
> +	tristate "Virtual concatenated MTD devices"
> +	help
> +	  This driver allows creation of a virtual MTD device, which
> +	  concatenates multiple physical MTD devices into a single one.
> +	  This is useful to create partitions bigger than the underlying
> +	  physical chips by allowing cross-chip boundaries.
> +
>  source "drivers/mtd/chips/Kconfig"
>  
>  source "drivers/mtd/maps/Kconfig"
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 58fc327a5276..c7ee13368a66 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_SSFDC)		+= ssfdc.o
>  obj-$(CONFIG_SM_FTL)		+= sm_ftl.o
>  obj-$(CONFIG_MTD_OOPS)		+= mtdoops.o
>  obj-$(CONFIG_MTD_SWAP)		+= mtdswap.o
> +obj-$(CONFIG_MTD_VIRT_CONCAT)	+= mtd_virt_concat.o

Can we move that code to mtdconcat? After, it's just an extension to
the mtdconcat logic that extract the description from a DT instead of
expecting drivers to create the concatenation on their own.


> +
> +static int __init mtd_virt_concat_init(void)
> +{
> +	struct mtd_virt_concat_node *item;
> +	struct device_node *parts = NULL;
> +	int ret = 0, count;
> +
> +	/* List all the concatenations found in DT */
> +	do {
> +		parts = of_find_node_with_property(parts, CONCAT_PROP);
> +		if (!of_device_is_available(parts))
> +			continue;
> +
> +		count = of_count_phandle_with_args(parts, CONCAT_PROP, NULL);
> +		if (count < MIN_DEV_PER_CONCAT)
> +			continue;
> +
> +		ret = mtd_virt_concat_create_item(parts, count);
> +		if (ret) {
> +			of_node_put(parts);
> +			goto destroy_items;
> +		}
> +	} while (parts);
> +
> +	/* TODO: also parse the cmdline */
> +
> +	/* Create the concatenations */
> +	list_for_each_entry(item, &concat_list, head) {
> +		ret = mtd_virt_concat_create_join(item);
> +		if (ret)
> +			goto destroy_joins;
> +	}
> +
> +	return 0;
> +
> +destroy_joins:
> +	mtd_virt_concat_destroy_joins();
> +destroy_items:
> +	mtd_virt_concat_destroy_items();
> +
> +	return ret;
> +}
> +late_initcall(mtd_virt_concat_init);

Right now the solution assumes all drivers are statically linked. I'm
pretty sure we can support other cases if we use MTD notifiers to be
informed of MTD device is addition/removal.

> +
> +static void __exit mtd_virt_concat_exit(void)
> +{
> +	mtd_virt_concat_destroy_joins();
> +	mtd_virt_concat_destroy_items();
> +}
> +module_exit(mtd_virt_concat_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Bernhard Frauendienst <kernel@nospam.obeliks.de>");
> +MODULE_DESCRIPTION("Virtual concat MTD device driver");


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices

[Miquel Raynal]

Hi Rob,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 9 Dec
2019 11:35:06 +0100:

> On Wed, 27 Nov 2019 11:55:22 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Introduce a generic way to define concatenated MTD devices. This may
> > be very useful in the case of ie. stacked SPI-NOR. Partitions to
> > concatenate are described in an additional property of the partitions
> > subnode:
> > 
> >         flash0 {
> >                 partitions {
> >                         compatible = "fixed-partitions";
> >                         part-concat = <&flash0_part1>, <&flash1_part0>;
> > 
> > 			part0@0 {
> > 				label = "part0_0";
> > 				reg = <0x0 0x800000>;
> > 			};
> > 
> > 			flash0_part1: part1@800000 {
> > 				label = "part0_1";
> > 				reg = <0x800000 0x800000>;  
> 
> So, flash0_part1 and flash0_part2 will be created even though the user
> probably doesn't need them?
> 
> > 			};
> >                 };
> >         };
> > 
> >         flash1 {
> >                 partitions {
> >                         compatible = "fixed-partitions";
> > 
> > 			flash0_part1: part1@0 {
> > 				label = "part1_0";
> > 				reg = <0x0 0x800000>;
> > 			};
> > 
> > 			part0@800000 {
> > 				label = "part1_1";
> > 				reg = <0x800000 0x800000>;
> > 			};
> >                 };
> >         };  
> 
> IMHO this representation is far from intuitive. At first glance it's not
> obvious which partitions are linked together and what's the name of the
> resulting concatenated part. I definitely prefer the solution where we
> have a virtual device describing the concatenation. I also understand
> that this goes against the #1 DT rule: "DT only decribes HW blocks, not
> how they should be used/configured", but maybe we can find a compromise
> here, like moving this description to the /chosen node?
> 
> chosen {
> 	flash-arrays {
> 		/*
> 		 * my-flash-array is the MTD name if label is
> 		 * not present.
> 		 */
> 		my-flash-array {
> 			/*
> 			 * We could have
> 			 * compatible = "flash-array";
> 			 * but we can also do without it.
> 			 */
> 			label = "foo";
> 			flashes = <&flash1 &flash2 ...>;
> 			partitions {
> 				/* usual partition description. */
> 				...
> 			};
> 		};
> 	};
> };
> 
> Rob, what do you think?

Rob, I would really welcome your thoughts on this solution, having
something like a flash-array node in the /chosen/ node would avoid
creating dummy devices, keep the declarations of the physical nodes
tidy and have a very simple description.

Hope this compromise could fit!
 
> 
> > 
> > This is useful for boards where memory range has been extended with
> > the use of multiple flash chips as memory banks of a single MTD
> > device, with partitions spanning chip borders.
> > 
> > Suggested-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices

[Rob Herring]

On Mon, Dec 9, 2019 at 4:35 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Wed, 27 Nov 2019 11:55:22 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> > Introduce a generic way to define concatenated MTD devices. This may
> > be very useful in the case of ie. stacked SPI-NOR. Partitions to
> > concatenate are described in an additional property of the partitions
> > subnode:
> >
> >         flash0 {
> >                 partitions {
> >                         compatible = "fixed-partitions";
> >                         part-concat = <&flash0_part1>, <&flash1_part0>;
> >
> >                       part0@0 {
> >                               label = "part0_0";
> >                               reg = <0x0 0x800000>;
> >                       };
> >
> >                       flash0_part1: part1@800000 {
> >                               label = "part0_1";
> >                               reg = <0x800000 0x800000>;
>
> So, flash0_part1 and flash0_part2 will be created even though the user
> probably doesn't need them?

I don't follow?

>
> >                       };
> >                 };
> >         };
> >
> >         flash1 {
> >                 partitions {
> >                         compatible = "fixed-partitions";
> >
> >                       flash0_part1: part1@0 {
> >                               label = "part1_0";
> >                               reg = <0x0 0x800000>;
> >                       };
> >
> >                       part0@800000 {
> >                               label = "part1_1";
> >                               reg = <0x800000 0x800000>;
> >                       };
> >                 };
> >         };
>
> IMHO this representation is far from intuitive. At first glance it's not
> obvious which partitions are linked together and what's the name of the
> resulting concatenated part. I definitely prefer the solution where we
> have a virtual device describing the concatenation. I also understand
> that this goes against the #1 DT rule: "DT only decribes HW blocks, not
> how they should be used/configured", but maybe we can find a compromise
> here, like moving this description to the /chosen node?
>
> chosen {
>         flash-arrays {
>                 /*
>                  * my-flash-array is the MTD name if label is
>                  * not present.
>                  */
>                 my-flash-array {
>                         /*
>                          * We could have
>                          * compatible = "flash-array";
>                          * but we can also do without it.
>                          */
>                         label = "foo";
>                         flashes = <&flash1 &flash2 ...>;
>                         partitions {
>                                 /* usual partition description. */
>                                 ...
>                         };
>                 };
>         };
> };
>
> Rob, what do you think?

I don't think chosen is the right place to put all the partition
information. It's not something the bootloader configures.

This suffers from the same issue I have with the original proposal. It
will not work for existing s/w. There's only 1 logical partition that
concatenated. The rest of the partitions shouldn't need any special
handling. So we really only need some way to say 'link these 2
partitions into 1 logical partition'. Though perhaps one could want to
combine any number of physical partitions into logical partitions, but
then none of the proposals could support that. Then again, maybe
that's a userspace problem like with disks.

To throw out another option, what if the first device contains the
complete partitions for both devices with some property in one or both
devices pointing to the other device? That would make the partitions
in the 1st device still accessible to existing s/w (unless it bounds
checks the partitions).

Rob

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices

[Miquel Raynal]

Hi Rob,

Rob Herring <robh+dt@kernel.org> wrote on Tue, 14 Jan 2020 11:46:18
-0600:

> On Mon, Dec 9, 2019 at 4:35 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Wed, 27 Nov 2019 11:55:22 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >  
> > > Introduce a generic way to define concatenated MTD devices. This may
> > > be very useful in the case of ie. stacked SPI-NOR. Partitions to
> > > concatenate are described in an additional property of the partitions
> > > subnode:
> > >
> > >         flash0 {
> > >                 partitions {
> > >                         compatible = "fixed-partitions";
> > >                         part-concat = <&flash0_part1>, <&flash1_part0>;
> > >
> > >                       part0@0 {
> > >                               label = "part0_0";
> > >                               reg = <0x0 0x800000>;
> > >                       };
> > >
> > >                       flash0_part1: part1@800000 {
> > >                               label = "part0_1";
> > >                               reg = <0x800000 0x800000>;  
> >
> > So, flash0_part1 and flash0_part2 will be created even though the user
> > probably doesn't need them?  
> 
> I don't follow?

Well, one will have to create "fake" partitions in order to concatenate
them with this solution, instead of just concatenating the devices (in
the case where you want to concatenate the entire devices). But the real
debate is below, on the representation.

> 
> >  
> > >                       };
> > >                 };
> > >         };
> > >
> > >         flash1 {
> > >                 partitions {
> > >                         compatible = "fixed-partitions";
> > >
> > >                       flash0_part1: part1@0 {
> > >                               label = "part1_0";
> > >                               reg = <0x0 0x800000>;
> > >                       };
> > >
> > >                       part0@800000 {
> > >                               label = "part1_1";
> > >                               reg = <0x800000 0x800000>;
> > >                       };
> > >                 };
> > >         };  
> >
> > IMHO this representation is far from intuitive. At first glance it's not
> > obvious which partitions are linked together and what's the name of the
> > resulting concatenated part. I definitely prefer the solution where we
> > have a virtual device describing the concatenation. I also understand
> > that this goes against the #1 DT rule: "DT only decribes HW blocks, not
> > how they should be used/configured", but maybe we can find a compromise
> > here, like moving this description to the /chosen node?
> >
> > chosen {
> >         flash-arrays {
> >                 /*
> >                  * my-flash-array is the MTD name if label is
> >                  * not present.
> >                  */
> >                 my-flash-array {
> >                         /*
> >                          * We could have
> >                          * compatible = "flash-array";
> >                          * but we can also do without it.
> >                          */
> >                         label = "foo";
> >                         flashes = <&flash1 &flash2 ...>;
> >                         partitions {
> >                                 /* usual partition description. */
> >                                 ...
> >                         };
> >                 };
> >         };
> > };
> >
> > Rob, what do you think?  
> 
> I don't think chosen is the right place to put all the partition
> information. It's not something the bootloader configures.
> 
> This suffers from the same issue I have with the original proposal. It
> will not work for existing s/w. There's only 1 logical partition that

I don't get why it would not work? Current hardware will just not have
the concatenation support, that's all. How is this a problem?

> concatenated. The rest of the partitions shouldn't need any special
> handling. So we really only need some way to say 'link these 2
> partitions into 1 logical partition'. Though perhaps one could want to
> combine any number of physical partitions into logical partitions, but
> then none of the proposals could support that. Then again, maybe

Yes, the flash-array proposal supports having more than two
partitions/devices concatenated, it is also already supported by the
driver (you don't care about this, but I do :) ).

> that's a userspace problem like with disks.

I see one big issue with this solution: what about bootloaders?

The root cause for such idea is that, in my case, the 2 MTD devices are
too small to contain the images needed to boot. The perfect solution is
to merge the two devices virtually in one single device and let U-Boot
read it like one.

I need to have the same representation both in U-Boot and Linux, hence
a userspace tool and a kernel command line argument do not work, right?

> To throw out another option, what if the first device contains the
> complete partitions for both devices with some property in one or both
> devices pointing to the other device? That would make the partitions
> in the 1st device still accessible to existing s/w (unless it bounds
> checks the partitions).

From a coding perspective this is very difficult as bound checks are
done everywhere and lying about the boundaries is IMHO a bit complex.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices

[Rob Herring]

On Tue, Jan 14, 2020 at 12:11 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Rob,
>
> Rob Herring <robh+dt@kernel.org> wrote on Tue, 14 Jan 2020 11:46:18
> -0600:
>
> > On Mon, Dec 9, 2019 at 4:35 AM Boris Brezillon
> > <boris.brezillon@collabora.com> wrote:
> > >
> > > On Wed, 27 Nov 2019 11:55:22 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > > Introduce a generic way to define concatenated MTD devices. This may
> > > > be very useful in the case of ie. stacked SPI-NOR. Partitions to
> > > > concatenate are described in an additional property of the partitions
> > > > subnode:
> > > >
> > > >         flash0 {
> > > >                 partitions {
> > > >                         compatible = "fixed-partitions";
> > > >                         part-concat = <&flash0_part1>, <&flash1_part0>;
> > > >
> > > >                       part0@0 {
> > > >                               label = "part0_0";
> > > >                               reg = <0x0 0x800000>;
> > > >                       };
> > > >
> > > >                       flash0_part1: part1@800000 {
> > > >                               label = "part0_1";
> > > >                               reg = <0x800000 0x800000>;
> > >
> > > So, flash0_part1 and flash0_part2 will be created even though the user
> > > probably doesn't need them?
> >
> > I don't follow?
>
> Well, one will have to create "fake" partitions in order to concatenate
> them with this solution, instead of just concatenating the devices (in
> the case where you want to concatenate the entire devices). But the real
> debate is below, on the representation.

So concatenating multiple devices without partitions defined in DT? To
support that, then we need to link flash nodes rather than partition
nodes.

> > > >                       };
> > > >                 };
> > > >         };
> > > >
> > > >         flash1 {
> > > >                 partitions {
> > > >                         compatible = "fixed-partitions";
> > > >
> > > >                       flash0_part1: part1@0 {
> > > >                               label = "part1_0";
> > > >                               reg = <0x0 0x800000>;
> > > >                       };
> > > >
> > > >                       part0@800000 {
> > > >                               label = "part1_1";
> > > >                               reg = <0x800000 0x800000>;
> > > >                       };
> > > >                 };
> > > >         };
> > >
> > > IMHO this representation is far from intuitive. At first glance it's not
> > > obvious which partitions are linked together and what's the name of the
> > > resulting concatenated part. I definitely prefer the solution where we
> > > have a virtual device describing the concatenation. I also understand
> > > that this goes against the #1 DT rule: "DT only decribes HW blocks, not
> > > how they should be used/configured", but maybe we can find a compromise
> > > here, like moving this description to the /chosen node?
> > >
> > > chosen {
> > >         flash-arrays {
> > >                 /*
> > >                  * my-flash-array is the MTD name if label is
> > >                  * not present.
> > >                  */
> > >                 my-flash-array {
> > >                         /*
> > >                          * We could have
> > >                          * compatible = "flash-array";
> > >                          * but we can also do without it.
> > >                          */
> > >                         label = "foo";
> > >                         flashes = <&flash1 &flash2 ...>;
> > >                         partitions {
> > >                                 /* usual partition description. */
> > >                                 ...
> > >                         };
> > >                 };
> > >         };
> > > };
> > >
> > > Rob, what do you think?
> >
> > I don't think chosen is the right place to put all the partition
> > information. It's not something the bootloader configures.
> >
> > This suffers from the same issue I have with the original proposal. It
> > will not work for existing s/w. There's only 1 logical partition that
>
> I don't get why it would not work? Current hardware will just not have
> the concatenation support, that's all. How is this a problem?

No one has multiple flash devices on any h/w already? If I already
have a working system that can load and boot a kernel off an MTD
partition, but could benefit from concatenating other partitions (i.e.
rootfs), why should I have to modify my bootloader(s)?

> > concatenated. The rest of the partitions shouldn't need any special
> > handling. So we really only need some way to say 'link these 2
> > partitions into 1 logical partition'. Though perhaps one could want to
> > combine any number of physical partitions into logical partitions, but
> > then none of the proposals could support that. Then again, maybe
>
> Yes, the flash-array proposal supports having more than two
> partitions/devices concatenated, it is also already supported by the
> driver (you don't care about this, but I do :) ).

I meant for N devices, you'd only have at most N-1 concatenated
partitions as there are N-1 device boundaries. Whereas you could
define any partition could be combined with any other number of
partitions regardless of where they lie. The types of things you can
do with LVM and disk partitions is what I'm thinking of.

> > that's a userspace problem like with disks.
>
> I see one big issue with this solution: what about bootloaders?

Similar to how it works with disks. Bootloaders (at a minimum)
understand physical partitions. Later stages understand logical
partitions.

> The root cause for such idea is that, in my case, the 2 MTD devices are
> too small to contain the images needed to boot. The perfect solution is
> to merge the two devices virtually in one single device and let U-Boot
> read it like one.

Got any actual h/w that the flash devices are smaller than a
kernel+ramdisk yet had the $ and board space to put multiple devices
down?

What about the stages before u-boot? Assume they all use DT, why
require adding this support before a stage that actually needs it.

> I need to have the same representation both in U-Boot and Linux, hence
> a userspace tool and a kernel command line argument do not work, right?

I never said u-boot can't gain support for this, it's just requiring
it to from the start even if you only wanted to have a concatenated
partition for your rootfs or non-boot partition. Only the kernel would
need to understand concatenating partitions vs. every component that
reads the partitions.

Rob

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/