[PATCH v12 0/3] mtd: Add support for stacked memories

[PATCH v12 0/3] mtd: Add support for stacked memories

[Amit Kumar Mahapatra]

This patch series is the next version of [1]. After a series of discussions 
on [1], we have decided to:
1/ Send separate patch series for stacked and parallel support, starting 
with the stacked series first.
2/ Add stacked support by enhancing the existing mtd-concat driver.

To add stacked support via mtd-concat, MTD device concatenation needs to be 
made more generic, and I found [2] to be a suitable approach for this.

As background, a few years ago, Bernhard Frauendienst initiated an effort 
[3] to achieve the same, which was later adapted by Miquel [2] to introduce 
stacked mode support. In this approach, partitions to be concatenated were 
specified using a DT property "part-concat" within the partitions 
definition, allowing two MTD devices to function as a single larger one in 
order to be able to define partitions across chip boundaries. However, the 
bindings were not accepted. As a result, the mtd-concat approach was 
dropped, and alternative DT bindings were introduced [4][5][6], describing 
the two flash devices as one. Corresponding SPI core changes to support 
these bindings were later added [7].

While integrating stacked mode support into SPI-NOR, Tudor provided 
additional feedback, leading to discussions about updating the existing 
DT bindings. To address this, I sent an RFC [8] to initiate discussions on 
adapting the DT bindings as suggested by Miquel in [2]. Following that, 
I am now submitting this patch series that updates the virtual concat 
driver referenced in [2] along with some minor mtdcore changes. 
Since I have taken ownership of this effort, I have included Bernhard and 
Miquel under the "Suggested-by" tag.

[1] https://lore.kernel.org/all/20231125092137.2948-1-amit.kumar-mahapatra@amd.com/
[2] https://lore.kernel.org/linux-mtd/20191127105522.31445-1-miquel.raynal@bootlin.com/
[3] https://lwn.net/ml/linux-kernel/20180907173515.19990-1-kernel@nospam.obeliks.de/
[4] https://github.com/torvalds/linux/commit/f89504300e94524d5d5846ff8b728592ac72cec4
[5] https://github.com/torvalds/linux/commit/eba5368503b4291db7819512600fa014ea17c5a8
[6] https://github.com/torvalds/linux/commit/e2edd1b64f1c79e8abda365149ed62a2a9a494b4
[7]https://github.com/torvalds/linux/commit/4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b
[8] https://lore.kernel.org/all/20241026075347.580858-1-amit.kumar-mahapatra@amd.com/
---
BRANCH: for-next

Changes in v12:
 - Add stacked mode support throught mtd-concat driver.

Changes in v11:
- Rebased patch series on top of latest for-next branch.
- Added a new patch(1/10) to replace spi->chip_select with
  spi_get_chipselect() call in tps6594-spi.c.
- Added a new patch(2/10) to replace spi->chip_select with
  spi_get_chipseletc() call in cs35l56_hda_spi.c.
- In spi.c initialized unused CS[] to 0xff and spi->cs_index_mask
  to 0x01 in all flows.
- Updated spi_dev_check() to compare the CS of old spi device with
  the new spi device CS.
- Updated cover letter description to add information regarding GPIO CS
  testing and added Stefen's Tested-by tag in 3/10 patch.

Changes in v10:
 - Rebased patch series on top of latest for-next branch and fixed
   merge conflicts.

Changes in v9:
- Updated 1/8 patch description to add an high-level overview of
  parallel(multi-cs) & stacked design.
- Initialized all unused CS to 0xFF.
- Moved CS check from spi_add_device() to __spi_add_device().
- Updated __spi_add_device() to check to make sure that multiple logical CS
  don't map to the same physical CS and same physical CS doesn't map to
  different logical CS.
- Updated 1/8, 5/8 & 7/8 to support arbitrary number of flash devices
  connected in parallel or stacked mode.
- Updated documentation for chip_select.
- Added a new spi-nor structure member nor->num_flash to keep track of the
  number of flashes connected.
- Added a new patch in the series 4/8 to move write_enable call just before
  spi_mem ops call in SPI-NOR.
- Added comments in SPI core & SPI-NOR.
- Rebased the patch series on top of the latest for-next branch.

Changes in v8:
- Updated __spi_add_device() and spi_set_cs() to fix spi driver failure
  with GPIO CS.
- Rebased the patch series on top of latest for-next branch and fixed
  merge conflicts.
- Updated cover letter description to add information regarding GPIO CS
  testing and request Stefan to provide his Tested-by tag for 1/7 patch.
- Updated 1/7 patch description.

Changes in v7:
- Updated spi_dev_check() to avoid failures for spi driver GPIO CS and
  moved the error message from __spi_add_device() to spi_dev_check().
- Resolved code indentation issue in spi_set_cs().
- In spi_set_cs() call spi_delay_exec( ) once if the controller supports
  multi cs with both the CS backed by GPIO.
- Updated __spi_validate()to add checks for both the GPIO CS.
- Replaced cs_index_mask bit mask with SPI_CS_CNT_MAX.
- Updated struct spi_controller to represent multi CS capability of the
  spi controller through a flag bit SPI_CONTROLLER_MULTI_CS instead of
  a boolen structure member "multi_cs_cap".
- Updated 1/7 patch description .

Changes in v6:
- Rebased on top of latest v6.3-rc1 and fixed merge conflicts in
  spi-mpc512x-psc.c, sfdp.c, spansion.c files and removed spi-omap-100k.c.
- Updated spi_dev_check( ) to reject new devices if any one of the
  chipselect is used by another device.

Changes in v5:
- Rebased the patches on top of v6.3-rc1 and fixed the merge conflicts.
- Fixed compilation warnings in spi-sh-msiof.c with shmobile_defconfig

Changes in v4:
- Fixed build error in spi-pl022.c file - reported by Mark.
- Fixed build error in spi-sn-f-ospi.c file.
- Added Reviewed-by: Serge Semin <fancer.lancer@gmail.com> tag.
- Added two more patches to replace spi->chip_select with API calls in
  mpc832x_rdb.c & cs35l41_hda_spi.c files.

Changes in v3:
- Rebased the patches on top of
  https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
- Added a patch to convert spi_nor_otp_region_len(nor) &
  spi_nor_otp_n_regions(nor) macros into inline functions
- Added Reviewed-by & Acked-by tags

Changes in v2:
- Rebased the patches on top of v6.2-rc1
- Created separate patch to add get & set APIs for spi->chip_select &
  spi->cs_gpiod, and replaced all spi->chip_select and spi->cs_gpiod
  references with the API calls.
- Created separate patch to add get & set APIs for nor->params.
---

Amit Kumar Mahapatra (3):
  dt-bindings: mtd: Describe MTD partitions concatenation
  mtd: Move struct mtd_concat definition to header file
  mtd: Add driver for concatenating devices

 .../bindings/mtd/partitions/partition.yaml    |  17 ++
 drivers/mtd/Kconfig                           |   8 +
 drivers/mtd/Makefile                          |   1 +
 drivers/mtd/mtd_virt_concat.c                 | 254 ++++++++++++++++++
 drivers/mtd/mtdconcat.c                       |  12 -
 drivers/mtd/mtdcore.c                         |   7 +
 drivers/mtd/mtdpart.c                         |   6 +
 include/linux/mtd/concat.h                    |  36 +++
 8 files changed, 329 insertions(+), 12 deletions(-)
 create mode 100644 drivers/mtd/mtd_virt_concat.c

-- 
2.34.1

[PATCH v12 1/3] dt-bindings: mtd: Describe MTD partitions concatenation

[Amit Kumar Mahapatra]

The AMD QSPI controller supports an advanced connection modes called
Stacked mode which allow the controller to treat two different flashes
as one storage.

In Stacked connection mode flashes share the same SPI bus, but different CS
line, controller driver asserts the CS of the flash to which it needs to
communicate. Stacked mode is a software abstraction rather than a
controller feature or capability. At any given time, the controller
communicates with one of the two connected flash devices, as determined by
the requested address and data length. If an operation starts on one flash
and ends on the other, the mtd layer needs to split it into two separate
operations and adjust the data length accordingly. For more information on
the modes please feel free to go through the controller flash interface
below [1].

Introduce new DT property to specify which partitions are concatenated to
each other.

    flash@0 {
            reg = <0>;
            partitions {
                    compatible = "fixed-partitions";
                    part-concat = <&flash0_part1>, <&flash1_part0>;

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

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

    flash@1 {
            reg = <1>;
            partitions {
                    compatible = "fixed-partitions";

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

                    flash1_part1: part1@800000 {
                            label = "part1_1";
                            reg = <0x800000 0x800000>;
                    };
            };
    };

The partitions that gets created are
part0_0
part1_1
part0_1-part1_0-concat

[1] https://docs.amd.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Device-Interface

Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
---
 .../bindings/mtd/partitions/partition.yaml      | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/partitions/partition.yaml b/Documentation/devicetree/bindings/mtd/partitions/partition.yaml
index 80d0452a2a33..f77fef368211 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/partition.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/partition.yaml
@@ -57,6 +57,12 @@ properties:
       user space from
     type: boolean
 
+  part-concat:
+    description: List of MTD partitions phandles that should be concatenated.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 2
+    maxItems: 16
+
   align:
     $ref: /schemas/types.yaml#/definitions/uint32
     minimum: 2
@@ -125,6 +131,7 @@ examples:
         compatible = "fixed-partitions";
         #address-cells = <1>;
         #size-cells = <1>;
+        part-concat = <&part0>, <&part1>;
 
         partition@100000 {
             compatible = "u-boot";
@@ -138,4 +145,14 @@ examples:
             reg = <0x200000 0x100000>;
             align = <0x4000>;
         };
+
+        part0: partition@400000 {
+            label = "part0_0";
+            reg = <0x400000 0x100000>;
+        };
+
+        part1: partition@800000 {
+            label = "part0_1";
+            reg = <0x800000 0x800000>;
+        };
     };
-- 
2.34.1

[PATCH v12 2/3] mtd: Move struct mtd_concat definition to header file

[Amit Kumar Mahapatra]

To enable a more generic approach for concatenating MTD devices,
struct mtd_concat should be accessible beyond the mtdconcat driver.
Therefore, the definition is being moved to a header file.

Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
---
 drivers/mtd/mtdconcat.c    | 12 ------------
 include/linux/mtd/concat.h | 12 ++++++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index f56f44aa8625..6f2aaceac669 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -20,18 +20,6 @@
 
 #include <asm/div64.h>
 
-/*
- * Our storage structure:
- * Subdev points to an array of pointers to struct mtd_info objects
- * which is allocated along with this structure
- *
- */
-struct mtd_concat {
-	struct mtd_info mtd;
-	int num_subdev;
-	struct mtd_info **subdev;
-};
-
 /*
  * how to calculate the size required for the above structure,
  * including the pointer array subdev points to:
diff --git a/include/linux/mtd/concat.h b/include/linux/mtd/concat.h
index d6f653e07426..b42d9af87c4e 100644
--- a/include/linux/mtd/concat.h
+++ b/include/linux/mtd/concat.h
@@ -9,6 +9,18 @@
 #define MTD_CONCAT_H
 
 
+/*
+ * Our storage structure:
+ * Subdev points to an array of pointers to struct mtd_info objects
+ * which is allocated along with this structure
+ *
+ */
+struct mtd_concat {
+	struct mtd_info mtd;
+	int num_subdev;
+	struct mtd_info **subdev;
+};
+
 struct mtd_info *mtd_concat_create(
     struct mtd_info *subdev[],  /* subdevices to concatenate */
     int num_devs,               /* number of subdevices      */
-- 
2.34.1

[PATCH v12 3/3] mtd: Add driver for concatenating devices

[Amit Kumar Mahapatra]

Introducing CONFIG_VIRT_CONCAT to separate the legacy flow from the new
approach, where individual partitions within a concatenated partition are
not registered, as they are likely not needed by the user.

Solution is focusing on fixed-partitions description only because it
depends on device boundaries.

Suggested-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
---
 drivers/mtd/Kconfig           |   8 ++
 drivers/mtd/Makefile          |   1 +
 drivers/mtd/mtd_virt_concat.c | 254 ++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdcore.c         |   7 +
 drivers/mtd/mtdpart.c         |   6 +
 include/linux/mtd/concat.h    |  24 ++++
 6 files changed, 300 insertions(+)
 create mode 100644 drivers/mtd/mtd_virt_concat.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 796a2eccbef0..3dade7c469df 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -206,6 +206,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
+	  The driver enables the creation of a virtual MTD device
+	  by concatenating multiple physical MTD devices into a single
+	  entity. This allows for the creation of partitions larger than
+	  the individual physical chips, extending across chip boundaries.
+
 source "drivers/mtd/chips/Kconfig"
 
 source "drivers/mtd/maps/Kconfig"
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 593d0593a038..d1d577f89a22 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SM_FTL)		+= sm_ftl.o
 obj-$(CONFIG_MTD_OOPS)		+= mtdoops.o
 obj-$(CONFIG_MTD_PSTORE)	+= mtdpstore.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..3de67305a09b
--- /dev/null
+++ b/drivers/mtd/mtd_virt_concat.c
@@ -0,0 +1,254 @@
+// 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/mtd.h>
+#include "mtdcore.h"
+#include <linux/mtd/partitions.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/mtd/concat.h>
+
+#define CONCAT_PROP "part-concat"
+#define MIN_DEV_PER_CONCAT 2
+
+static LIST_HEAD(concat_node_list);
+
+/**
+ * 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_concat *concat;
+};
+
+static void mtd_virt_concat_put_mtd_devices(struct mtd_concat *concat)
+{
+	int i;
+
+	for (i = 0; i < concat->num_subdev; i++)
+		put_mtd_device(concat->subdev[i]);
+}
+
+static void mtd_virt_concat_destroy_joins(void)
+{
+	struct mtd_virt_concat_node *item, *tmp;
+	struct mtd_info *mtd;
+
+	list_for_each_entry_safe(item, tmp, &concat_node_list, head) {
+		mtd = &item->concat->mtd;
+		if (item->concat) {
+			mtd_device_unregister(mtd);
+			kfree(mtd->name);
+			mtd_concat_destroy(mtd);
+			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;
+	struct mtd_concat *concat;
+	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);
+
+	concat = kzalloc(sizeof(*concat), GFP_KERNEL);
+	if (!concat) {
+		kfree(item);
+		return -ENOMEM;
+	}
+
+	concat->subdev = kcalloc(count, sizeof(*concat->subdev), GFP_KERNEL);
+	if (!concat->subdev) {
+		kfree(item);
+		kfree(concat);
+		return -ENOMEM;
+	}
+	item->concat = concat;
+
+	list_add_tail(&item->head, &concat_node_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_node_list, head) {
+		for (i = 0; i < item->count; i++)
+			of_node_put(item->nodes[i]);
+
+		kfree(item->nodes);
+		kfree(item);
+	}
+}
+
+bool mtd_virt_concat_add(struct mtd_info *mtd)
+{
+	struct mtd_virt_concat_node *item;
+	struct mtd_concat *concat;
+	int idx;
+
+	list_for_each_entry(item, &concat_node_list, head) {
+		concat = item->concat;
+		if (item->count == concat->num_subdev)
+			return false;
+
+		for (idx = 0; idx < item->count; idx++) {
+			if (item->nodes[idx] == mtd->dev.of_node) {
+				concat->subdev[concat->num_subdev++] = mtd;
+				return true;
+			}
+		}
+	}
+	return false;
+}
+EXPORT_SYMBOL_GPL(mtd_virt_concat_add);
+
+int mtd_virt_concat_node_create(void)
+{
+	struct mtd_concat *concat;
+	struct device_node *parts = NULL;
+	int ret = 0, count;
+
+	if (!list_empty(&concat_node_list))
+		return 0;
+
+	/* 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);
+
+	concat = kzalloc(sizeof(*concat), GFP_KERNEL);
+	if (!concat) {
+		ret = -ENOMEM;
+		of_node_put(parts);
+		goto destroy_items;
+	}
+
+	concat->subdev = kcalloc(count, sizeof(*concat->subdev), GFP_KERNEL);
+	if (!concat->subdev) {
+		kfree(concat);
+		ret = -ENOMEM;
+		of_node_put(parts);
+		goto destroy_items;
+	}
+
+	return count;
+
+destroy_items:
+	mtd_virt_concat_destroy_items();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(mtd_virt_concat_node_create);
+
+static int __init mtd_virt_concat_create_join(void)
+{
+	struct mtd_virt_concat_node *item;
+	struct mtd_concat *concat;
+	struct mtd_info *mtd;
+	ssize_t name_sz;
+	char *name;
+	int ret;
+
+	list_for_each_entry(item, &concat_node_list, head) {
+		concat = item->concat;
+		mtd = &concat->mtd;
+		/* Create the virtual device */
+		name_sz = snprintf(NULL, 0, "%s-%s%s-concat",
+				   concat->subdev[0]->name,
+				   concat->subdev[1]->name,
+				   concat->num_subdev > MIN_DEV_PER_CONCAT ?
+				   "-+" : "");
+		name = kmalloc(name_sz + 1, GFP_KERNEL);
+		if (!name) {
+			mtd_virt_concat_put_mtd_devices(concat);
+			return -ENOMEM;
+		}
+
+		sprintf(name, "%s-%s%s-concat",
+			concat->subdev[0]->name,
+			concat->subdev[1]->name,
+			concat->num_subdev > MIN_DEV_PER_CONCAT ?
+			"-+" : "");
+
+		mtd = mtd_concat_create(concat->subdev, concat->num_subdev, name);
+		if (!mtd) {
+			kfree(name);
+			return -ENXIO;
+		}
+
+		/* Arbitrary set the first device as parent */
+		mtd->dev.parent = concat->subdev[0]->dev.parent;
+		mtd->dev = concat->subdev[0]->dev;
+
+		/* Register the platform device */
+		ret = mtd_device_register(mtd, NULL, 0);
+		if (ret)
+			goto destroy_concat;
+	}
+
+	return 0;
+
+destroy_concat:
+	mtd_concat_destroy(mtd);
+
+	return ret;
+}
+
+late_initcall(mtd_virt_concat_create_join);
+
+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");
+MODULE_AUTHOR("Bernhard Frauendienst <kernel@nospam.obeliks.de>");
+MODULE_AUTHOR("Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>");
+MODULE_DESCRIPTION("Virtual concat MTD device driver");
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 724f917f91ba..2264fe81810f 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -34,6 +34,7 @@
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
+#include <linux/mtd/concat.h>
 
 #include "mtdcore.h"
 
@@ -1067,6 +1068,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			goto out;
 	}
 
+	if (IS_ENABLED(CONFIG_MTD_VIRT_CONCAT)) {
+		ret = mtd_virt_concat_node_create();
+		if (ret < 0)
+			goto out;
+	}
+
 	/* Prefer parsed partitions over driver-provided fallback */
 	ret = parse_mtd_partitions(mtd, types, parser_data);
 	if (ret == -EPROBE_DEFER)
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 6811a714349d..4f50e14b96dd 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -18,6 +18,7 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/mtd/concat.h>
 
 #include "mtdcore.h"
 
@@ -409,6 +410,11 @@ int add_mtd_partitions(struct mtd_info *parent,
 			goto err_del_partitions;
 		}
 
+		if (IS_ENABLED(CONFIG_MTD_VIRT_CONCAT)) {
+			if (mtd_virt_concat_add(child))
+				continue;
+		}
+
 		mutex_lock(&master->master.partitions_lock);
 		list_add_tail(&child->part.node, &parent->partitions);
 		mutex_unlock(&master->master.partitions_lock);
diff --git a/include/linux/mtd/concat.h b/include/linux/mtd/concat.h
index b42d9af87c4e..65c9a18774f6 100644
--- a/include/linux/mtd/concat.h
+++ b/include/linux/mtd/concat.h
@@ -28,5 +28,29 @@ struct mtd_info *mtd_concat_create(
 
 void mtd_concat_destroy(struct mtd_info *mtd);
 
+/**
+ * mtd_virt_concat_node_create - Create a component for concatenation
+ *
+ * Returns a positive number representing the no. of devices found for
+ * concatenation, or a negative error code.
+ *
+ * List all the devices for concatenations found in DT and create a
+ * component for concatenation.
+ */
+int mtd_virt_concat_node_create(void);
+
+/**
+ * mtd_virt_concat_add - add mtd_info object to the list of subdevices for concatenation
+ * @mtd: pointer to new MTD device info structure
+ *
+ * Returns true if the mtd_info object is added successfully else returns false.
+ *
+ * The mtd_info object is added to the list of subdevices for concatenation.
+ * It returns true if a match is found, and false if all subdevices have
+ * already been added or if the mtd_info object does not match any of the
+ * intended MTD devices.
+ */
+bool mtd_virt_concat_add(struct mtd_info *mtd);
+
 #endif
 
-- 
2.34.1

Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Markus Elfring]

I suggest to improve implementation details another bit at a few source code places.

Examples:
…
> +++ b/drivers/mtd/mtd_virt_concat.c
> @@ -0,0 +1,254 @@
…
> +static int mtd_virt_concat_create_item(struct device_node *parts,
> +				       unsigned int count)
> +{

+	struct mtd_virt_concat_node *item __free(kfree) = kzalloc(sizeof(*item), GFP_KERNEL);


…
> +int mtd_virt_concat_node_create(void)
> +{
…

+e_nomem:
+	ret = -ENOMEM;
+put_parts:
+	of_node_put(parts);

> +destroy_items:
> +	mtd_virt_concat_destroy_items();
> +
> +	return ret;
> +}
…


Regards,
Markus

Re: [PATCH v12 1/3] dt-bindings: mtd: Describe MTD partitions concatenation

[Rob Herring]

On Wed, Feb 05, 2025 at 07:07:28PM +0530, Amit Kumar Mahapatra wrote:
> The AMD QSPI controller supports an advanced connection modes called
> Stacked mode which allow the controller to treat two different flashes
> as one storage.
> 
> In Stacked connection mode flashes share the same SPI bus, but different CS
> line, controller driver asserts the CS of the flash to which it needs to
> communicate. Stacked mode is a software abstraction rather than a
> controller feature or capability. At any given time, the controller
> communicates with one of the two connected flash devices, as determined by
> the requested address and data length. If an operation starts on one flash
> and ends on the other, the mtd layer needs to split it into two separate
> operations and adjust the data length accordingly. For more information on
> the modes please feel free to go through the controller flash interface
> below [1].
> 
> Introduce new DT property to specify which partitions are concatenated to
> each other.
> 
>     flash@0 {
>             reg = <0>;
>             partitions {
>                     compatible = "fixed-partitions";
>                     part-concat = <&flash0_part1>, <&flash1_part0>;
> 
>                     flash0_part0: part0@0 {
>                             label = "part0_0";
>                             reg = <0x0 0x800000>;
>                     };
> 
>                     flash0_part1: part1@800000 {
>                             label = "part0_1";
>                             reg = <0x800000 0x800000>;
>                     };
>             };
>     };
> 
>     flash@1 {
>             reg = <1>;
>             partitions {
>                     compatible = "fixed-partitions";
> 
>                     flash1_part0: part1@0 {
>                             label = "part1_0";
>                             reg = <0x0 0x800000>;
>                     };
> 
>                     flash1_part1: part1@800000 {
>                             label = "part1_1";
>                             reg = <0x800000 0x800000>;
>                     };
>             };
>     };
> 
> The partitions that gets created are
> part0_0
> part1_1
> part0_1-part1_0-concat

'part-concat' doesn't work if you have multiple sets of partitions you 
want to concatenate.

I think you need something like 'prev-partition' or 'next-partition' in 
the partition nodes to create a linked list of partitions. Hopefully, 
you don't need both properties, but you do have to scan everything to 
figure out which ones are concatenated or not. For example, no property 
can mean not concatenated or last partition if you use 'next-partition'. 

Rob

Re: [PATCH v12 1/3] dt-bindings: mtd: Describe MTD partitions concatenation

[Miquel Raynal]

Hi,

>> The partitions that gets created are
>> part0_0
>> part1_1
>> part0_1-part1_0-concat
>
> 'part-concat' doesn't work if you have multiple sets of partitions you 
> want to concatenate.
>
> I think you need something like 'prev-partition' or 'next-partition' in 
> the partition nodes to create a linked list of partitions. Hopefully, 
> you don't need both properties, but you do have to scan everything to 
> figure out which ones are concatenated or not. For example, no property 
> can mean not concatenated or last partition if you use 'next-partition'. 

Out of curiosity, would the chosen node be eligible as a central place
where to look at?

Thanks,
Miquèl

Re: [PATCH v12 1/3] dt-bindings: mtd: Describe MTD partitions concatenation

[Rob Herring]

On Wed, Feb 12, 2025 at 09:25:53AM +0100, Miquel Raynal wrote:
> Hi,
> 
> >> The partitions that gets created are
> >> part0_0
> >> part1_1
> >> part0_1-part1_0-concat
> >
> > 'part-concat' doesn't work if you have multiple sets of partitions you 
> > want to concatenate.
> >
> > I think you need something like 'prev-partition' or 'next-partition' in 
> > the partition nodes to create a linked list of partitions. Hopefully, 
> > you don't need both properties, but you do have to scan everything to 
> > figure out which ones are concatenated or not. For example, no property 
> > can mean not concatenated or last partition if you use 'next-partition'. 
> 
> Out of curiosity, would the chosen node be eligible as a central place
> where to look at?

Why would you need that?

Rob

Re: [PATCH v12 1/3] dt-bindings: mtd: Describe MTD partitions concatenation

[Miquel Raynal]

On 12/02/2025 at 10:06:59 -06, Rob Herring <robh@kernel.org> wrote:

> On Wed, Feb 12, 2025 at 09:25:53AM +0100, Miquel Raynal wrote:
>> Hi,
>> 
>> >> The partitions that gets created are
>> >> part0_0
>> >> part1_1
>> >> part0_1-part1_0-concat
>> >
>> > 'part-concat' doesn't work if you have multiple sets of partitions you 
>> > want to concatenate.
>> >
>> > I think you need something like 'prev-partition' or 'next-partition' in 
>> > the partition nodes to create a linked list of partitions. Hopefully, 
>> > you don't need both properties, but you do have to scan everything to 
>> > figure out which ones are concatenated or not. For example, no property 
>> > can mean not concatenated or last partition if you use 'next-partition'. 
>> 
>> Out of curiosity, would the chosen node be eligible as a central place
>> where to look at?
>
> Why would you need that?

I'm talking about storing in a central place all the concatenated
partitions. Your proposal with "next-partition" works fine if we locate
it inside the 'partitions' node, but I feel like the 'part-concat'
instead was not fitting very well there. So I was wondering in this case
if moving the concatenation of the partitions would be eligible to the
chosen node, or if that's reserved to *very* few properties (and should
remain like that).

Thanks,
Miquèl

Re: [PATCH v12 1/3] dt-bindings: mtd: Describe MTD partitions concatenation

[Rob Herring]

On Wed, Feb 12, 2025 at 05:18:39PM +0100, Miquel Raynal wrote:
> On 12/02/2025 at 10:06:59 -06, Rob Herring <robh@kernel.org> wrote:
> 
> > On Wed, Feb 12, 2025 at 09:25:53AM +0100, Miquel Raynal wrote:
> >> Hi,
> >> 
> >> >> The partitions that gets created are
> >> >> part0_0
> >> >> part1_1
> >> >> part0_1-part1_0-concat
> >> >
> >> > 'part-concat' doesn't work if you have multiple sets of partitions you 
> >> > want to concatenate.
> >> >
> >> > I think you need something like 'prev-partition' or 'next-partition' in 
> >> > the partition nodes to create a linked list of partitions. Hopefully, 
> >> > you don't need both properties, but you do have to scan everything to 
> >> > figure out which ones are concatenated or not. For example, no property 
> >> > can mean not concatenated or last partition if you use 'next-partition'. 
> >> 
> >> Out of curiosity, would the chosen node be eligible as a central place
> >> where to look at?
> >
> > Why would you need that?
> 
> I'm talking about storing in a central place all the concatenated
> partitions. Your proposal with "next-partition" works fine if we locate
> it inside the 'partitions' node, but I feel like the 'part-concat'
> instead was not fitting very well there. So I was wondering in this case
> if moving the concatenation of the partitions would be eligible to the
> chosen node, or if that's reserved to *very* few properties (and should
> remain like that).

You would have to solve the same problem as this patchset which is how 
to support N sets of concatenated partitions.

In general though, we add new things to /chosen very carefully. It's 
usually "things the bootloader configured/enabled" which I don't think 
this qualifies as. 

Rob

Re: [PATCH v12 1/3] dt-bindings: mtd: Describe MTD partitions concatenation

[Miquel Raynal]

Hi Rob,

>> I'm talking about storing in a central place all the concatenated
>> partitions. Your proposal with "next-partition" works fine if we locate
>> it inside the 'partitions' node, but I feel like the 'part-concat'
>> instead was not fitting very well there. So I was wondering in this case
>> if moving the concatenation of the partitions would be eligible to the
>> chosen node, or if that's reserved to *very* few properties (and should
>> remain like that).
>
> You would have to solve the same problem as this patchset which is how 
> to support N sets of concatenated partitions.
>
> In general though, we add new things to /chosen very carefully. It's 
> usually "things the bootloader configured/enabled" which I don't think 
> this qualifies as.

Interesting, I didn't have this "things the bootloader did" explicit
case in mind.

Thanks!
Miquèl

Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Miquel Raynal]

On 05/02/2025 at 19:07:30 +0530, Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> wrote:

> Introducing CONFIG_VIRT_CONCAT to separate the legacy flow from the
> new

CONFIG_MTD_VIRT_CONCAT

> approach, where individual partitions within a concatenated partition are
> not registered, as they are likely not needed by the user.

I am not a big fan of this choice. We had issues with hiding things to
the user in the first place. Could we find a way to expose both the
original mtd devices as well as the virtually concatenated partitions?

> Solution is focusing on fixed-partitions description only because it
> depends on device boundaries.
>
> Suggested-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
> Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
> ---
>  drivers/mtd/Kconfig           |   8 ++
>  drivers/mtd/Makefile          |   1 +
>  drivers/mtd/mtd_virt_concat.c | 254 ++++++++++++++++++++++++++++++++++
>  drivers/mtd/mtdcore.c         |   7 +
>  drivers/mtd/mtdpart.c         |   6 +
>  include/linux/mtd/concat.h    |  24 ++++
>  6 files changed, 300 insertions(+)
>  create mode 100644 drivers/mtd/mtd_virt_concat.c
>
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 796a2eccbef0..3dade7c469df 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -206,6 +206,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
> +	  The driver enables the creation of a virtual MTD device

                                          of virtual MTD devices

> +	  by concatenating multiple physical MTD devices into a single
> +	  entity. This allows for the creation of partitions larger than
> +	  the individual physical chips, extending across chip boundaries.
> +

...

> +static int __init mtd_virt_concat_create_join(void)
> +{
> +	struct mtd_virt_concat_node *item;
> +	struct mtd_concat *concat;
> +	struct mtd_info *mtd;
> +	ssize_t name_sz;
> +	char *name;
> +	int ret;
> +
> +	list_for_each_entry(item, &concat_node_list, head) {
> +		concat = item->concat;
> +		mtd = &concat->mtd;
> +		/* Create the virtual device */
> +		name_sz = snprintf(NULL, 0, "%s-%s%s-concat",
> +				   concat->subdev[0]->name,
> +				   concat->subdev[1]->name,
> +				   concat->num_subdev > MIN_DEV_PER_CONCAT ?
> +				   "-+" : "");
> +		name = kmalloc(name_sz + 1, GFP_KERNEL);
> +		if (!name) {
> +			mtd_virt_concat_put_mtd_devices(concat);
> +			return -ENOMEM;
> +		}
> +
> +		sprintf(name, "%s-%s%s-concat",
> +			concat->subdev[0]->name,
> +			concat->subdev[1]->name,
> +			concat->num_subdev > MIN_DEV_PER_CONCAT ?
> +			"-+" : "");
> +
> +		mtd = mtd_concat_create(concat->subdev, concat->num_subdev, name);
> +		if (!mtd) {
> +			kfree(name);
> +			return -ENXIO;
> +		}
> +
> +		/* Arbitrary set the first device as parent */

Here we may face runtime PM issues. At some point the device model
expects a single parent per struct device, but here we have two. I do
not have any hints at the moment on how we could solve that.

> +		mtd->dev.parent = concat->subdev[0]->dev.parent;
> +		mtd->dev = concat->subdev[0]->dev;
> +
> +		/* Register the platform device */
> +		ret = mtd_device_register(mtd, NULL, 0);
> +		if (ret)
> +			goto destroy_concat;
> +	}
> +
> +	return 0;
> +
> +destroy_concat:
> +	mtd_concat_destroy(mtd);
> +
> +	return ret;
> +}
> +
> +late_initcall(mtd_virt_concat_create_join);

The current implementation does not support probe deferrals, I believe
it should be handled.

> +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");
> +MODULE_AUTHOR("Bernhard Frauendienst <kernel@nospam.obeliks.de>");
> +MODULE_AUTHOR("Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>");
> +MODULE_DESCRIPTION("Virtual concat MTD device driver");
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 724f917f91ba..2264fe81810f 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -34,6 +34,7 @@
>  
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
> +#include <linux/mtd/concat.h>
>  
>  #include "mtdcore.h"
>  
> @@ -1067,6 +1068,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			goto out;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_MTD_VIRT_CONCAT)) {

Maybe IS_REACHABLE() is more relevant?

> +		ret = mtd_virt_concat_node_create();
> +		if (ret < 0)
> +			goto out;
> +	}
> +
>  	/* Prefer parsed partitions over driver-provided fallback */
>  	ret = parse_mtd_partitions(mtd, types, parser_data);
>  	if (ret == -EPROBE_DEFER)

Thanks,
Miquèl

RE: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Mahapatra, Amit Kumar]

[AMD Official Use Only - AMD Internal Distribution Only]

Hello Miquel,

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Tuesday, March 18, 2025 9:23 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
> Cc: richard@nod.at; vigneshr@ti.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
> amitrkcian2002@gmail.com; Bernhard Frauendienst <kernel@nospam.obeliks.de>
> Subject: Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices
>
> On 05/02/2025 at 19:07:30 +0530, Amit Kumar Mahapatra <amit.kumar-
> mahapatra@amd.com> wrote:
>
> > Introducing CONFIG_VIRT_CONCAT to separate the legacy flow from the
> > new
>
> CONFIG_MTD_VIRT_CONCAT
>
> > approach, where individual partitions within a concatenated partition
> > are not registered, as they are likely not needed by the user.
>
> I am not a big fan of this choice. We had issues with hiding things to the user in the
> first place. Could we find a way to expose both the original mtd devices as well as
> the virtually concatenated partitions?

Sure, I think that can be done, but I took this approach to hide the
original devices because Boris mentioned in [1] that we are creating
the original partitions even though the user probably doesn't need
them. I believe he is right, as I can't think of any use case where
the user would require the individual devices instead of the
concatenated device.

[1] https://lore.kernel.org/linux-mtd/20191209113506.41341ed4@collabora.com/

>
> > Solution is focusing on fixed-partitions description only because it
> > depends on device boundaries.
> >
> > Suggested-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
> > Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
> > ---
> >  drivers/mtd/Kconfig           |   8 ++
> >  drivers/mtd/Makefile          |   1 +
> >  drivers/mtd/mtd_virt_concat.c | 254
> ++++++++++++++++++++++++++++++++++
> >  drivers/mtd/mtdcore.c         |   7 +
> >  drivers/mtd/mtdpart.c         |   6 +
> >  include/linux/mtd/concat.h    |  24 ++++
> >  6 files changed, 300 insertions(+)
> >  create mode 100644 drivers/mtd/mtd_virt_concat.c
> >
> > diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig index
> > 796a2eccbef0..3dade7c469df 100644
> > --- a/drivers/mtd/Kconfig
> > +++ b/drivers/mtd/Kconfig
> > @@ -206,6 +206,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
> > +     The driver enables the creation of a virtual MTD device
>
>                                           of virtual MTD devices
>
> > +     by concatenating multiple physical MTD devices into a single
> > +     entity. This allows for the creation of partitions larger than
> > +     the individual physical chips, extending across chip boundaries.
> > +
>
> ...
>
> > +static int __init mtd_virt_concat_create_join(void) {
> > +   struct mtd_virt_concat_node *item;
> > +   struct mtd_concat *concat;
> > +   struct mtd_info *mtd;
> > +   ssize_t name_sz;
> > +   char *name;
> > +   int ret;
> > +
> > +   list_for_each_entry(item, &concat_node_list, head) {
> > +           concat = item->concat;
> > +           mtd = &concat->mtd;
> > +           /* Create the virtual device */
> > +           name_sz = snprintf(NULL, 0, "%s-%s%s-concat",
> > +                              concat->subdev[0]->name,
> > +                              concat->subdev[1]->name,
> > +                              concat->num_subdev > MIN_DEV_PER_CONCAT
> ?
> > +                              "-+" : "");
> > +           name = kmalloc(name_sz + 1, GFP_KERNEL);
> > +           if (!name) {
> > +                   mtd_virt_concat_put_mtd_devices(concat);
> > +                   return -ENOMEM;
> > +           }
> > +
> > +           sprintf(name, "%s-%s%s-concat",
> > +                   concat->subdev[0]->name,
> > +                   concat->subdev[1]->name,
> > +                   concat->num_subdev > MIN_DEV_PER_CONCAT ?
> > +                   "-+" : "");
> > +
> > +           mtd = mtd_concat_create(concat->subdev, concat->num_subdev,
> name);
> > +           if (!mtd) {
> > +                   kfree(name);
> > +                   return -ENXIO;
> > +           }
> > +
> > +           /* Arbitrary set the first device as parent */
>
> Here we may face runtime PM issues. At some point the device model expects a
> single parent per struct device, but here we have two. I do not have any hints at the
> moment on how we could solve that.
>
> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
> > +           mtd->dev = concat->subdev[0]->dev;
> > +
> > +           /* Register the platform device */
> > +           ret = mtd_device_register(mtd, NULL, 0);
> > +           if (ret)
> > +                   goto destroy_concat;
> > +   }
> > +
> > +   return 0;
> > +
> > +destroy_concat:
> > +   mtd_concat_destroy(mtd);
> > +
> > +   return ret;
> > +}
> > +
> > +late_initcall(mtd_virt_concat_create_join);
>
> The current implementation does not support probe deferrals, I believe it should be
> handled.
>
> > +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");
> > +MODULE_AUTHOR("Bernhard Frauendienst <kernel@nospam.obeliks.de>");
> > +MODULE_AUTHOR("Amit Kumar Mahapatra <amit.kumar-
> mahapatra@amd.com>");
> > +MODULE_DESCRIPTION("Virtual concat MTD device driver");
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index
> > 724f917f91ba..2264fe81810f 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -34,6 +34,7 @@
> >
> >  #include <linux/mtd/mtd.h>
> >  #include <linux/mtd/partitions.h>
> > +#include <linux/mtd/concat.h>
> >
> >  #include "mtdcore.h"
> >
> > @@ -1067,6 +1068,12 @@ int mtd_device_parse_register(struct mtd_info *mtd,
> const char * const *types,
> >                     goto out;
> >     }
> >
> > +   if (IS_ENABLED(CONFIG_MTD_VIRT_CONCAT)) {
>
> Maybe IS_REACHABLE() is more relevant?

Agreed

Regards,
Amit
>
> > +           ret = mtd_virt_concat_node_create();
> > +           if (ret < 0)
> > +                   goto out;
> > +   }
> > +
> >     /* Prefer parsed partitions over driver-provided fallback */
> >     ret = parse_mtd_partitions(mtd, types, parser_data);
> >     if (ret == -EPROBE_DEFER)
>
> Thanks,
> Miquèl

Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Miquel Raynal]

On 19/03/2025 at 06:17:50 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hello Miquel,
>
>> -----Original Message-----
>> From: Miquel Raynal <miquel.raynal@bootlin.com>
>> Sent: Tuesday, March 18, 2025 9:23 PM
>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
>> Cc: richard@nod.at; vigneshr@ti.com; robh@kernel.org; krzk+dt@kernel.org;
>> conor+dt@kernel.org; linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
>> amitrkcian2002@gmail.com; Bernhard Frauendienst <kernel@nospam.obeliks.de>
>> Subject: Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices
>>
>> On 05/02/2025 at 19:07:30 +0530, Amit Kumar Mahapatra <amit.kumar-
>> mahapatra@amd.com> wrote:
>>
>> > Introducing CONFIG_VIRT_CONCAT to separate the legacy flow from the
>> > new
>>
>> CONFIG_MTD_VIRT_CONCAT
>>
>> > approach, where individual partitions within a concatenated partition
>> > are not registered, as they are likely not needed by the user.
>>
>> I am not a big fan of this choice. We had issues with hiding things to the user in the
>> first place. Could we find a way to expose both the original mtd devices as well as
>> the virtually concatenated partitions?
>
> Sure, I think that can be done, but I took this approach to hide the
> original devices because Boris mentioned in [1] that we are creating
> the original partitions even though the user probably doesn't need
> them. I believe he is right, as I can't think of any use case where
> the user would require the individual devices instead of the
> concatenated device.
>
> [1] https://lore.kernel.org/linux-mtd/20191209113506.41341ed4@collabora.com/

He was suggesting not to create the intermediate partitions, and I agree
it is not super relevant, but the flash devices themselves are relevant.

In this example:

concatenate = <&part2>, <&part3>;

flash@0 {
        part1 { }
        part2 { }
};

flash@0 {
        part3 { }
        part4 { }
};

part1, part2, part3, part4 are partitions, at least part1 and part4
should appear like mtd devices.
Boris was suggesting to not expose part2 and part3 individually when
concatenating them, I'm fine with that.

What I am saying, is that flash@0 and flash@1 shall be represented by
two mtd devices and not hidden/skipped.

Thanks,
Miquèl

RE: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Mahapatra, Amit Kumar]

[AMD Official Use Only - AMD Internal Distribution Only]

Hello Miquel,

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Tuesday, March 18, 2025 9:23 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
> Cc: richard@nod.at; vigneshr@ti.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
> amitrkcian2002@gmail.com; Bernhard Frauendienst <kernel@nospam.obeliks.de>
> Subject: Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices
>
> On 05/02/2025 at 19:07:30 +0530, Amit Kumar Mahapatra <amit.kumar-
> mahapatra@amd.com> wrote:
>
>
...

> > +static int __init mtd_virt_concat_create_join(void) {
> > +   struct mtd_virt_concat_node *item;
> > +   struct mtd_concat *concat;
> > +   struct mtd_info *mtd;
> > +   ssize_t name_sz;
> > +   char *name;
> > +   int ret;
> > +
> > +   list_for_each_entry(item, &concat_node_list, head) {
> > +           concat = item->concat;
> > +           mtd = &concat->mtd;
> > +           /* Create the virtual device */
> > +           name_sz = snprintf(NULL, 0, "%s-%s%s-concat",
> > +                              concat->subdev[0]->name,
> > +                              concat->subdev[1]->name,
> > +                              concat->num_subdev > MIN_DEV_PER_CONCAT
> ?
> > +                              "-+" : "");
> > +           name = kmalloc(name_sz + 1, GFP_KERNEL);
> > +           if (!name) {
> > +                   mtd_virt_concat_put_mtd_devices(concat);
> > +                   return -ENOMEM;
> > +           }
> > +
> > +           sprintf(name, "%s-%s%s-concat",
> > +                   concat->subdev[0]->name,
> > +                   concat->subdev[1]->name,
> > +                   concat->num_subdev > MIN_DEV_PER_CONCAT ?
> > +                   "-+" : "");
> > +
> > +           mtd = mtd_concat_create(concat->subdev, concat->num_subdev,
> name);
> > +           if (!mtd) {
> > +                   kfree(name);
> > +                   return -ENXIO;
> > +           }
> > +
> > +           /* Arbitrary set the first device as parent */
>
> Here we may face runtime PM issues. At some point the device model expects a
> single parent per struct device, but here we have two. I do not have any hints at the
> moment on how we could solve that.
>
> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
> > +           mtd->dev = concat->subdev[0]->dev;
> > +
> > +           /* Register the platform device */
> > +           ret = mtd_device_register(mtd, NULL, 0);
> > +           if (ret)
> > +                   goto destroy_concat;
> > +   }
> > +
> > +   return 0;
> > +
> > +destroy_concat:
> > +   mtd_concat_destroy(mtd);
> > +
> > +   return ret;
> > +}
> > +
> > +late_initcall(mtd_virt_concat_create_join);
>
> The current implementation does not support probe deferrals, I believe it should be
> handled.

I see that the parse_mtd_partitions() API can return -EPROBE_DEFER during
MTD device registration, but this behavior is specific to the
parse_qcomsmem_part parser. None of the other parsers appear to support
probe deferral. As discussed in RFC [1], the virtual concat feature is
purely a fixed-partition capability, and based on my understanding, the
fixed-partition parser does not support probe deferral.
Please let me know if you can think of any other probe deferral scenarios
that might impact the virtual concat driver.

[1] https://lore.kernel.org/all/87sermxme1.fsf@bootlin.com/

Regards,
Amit
...
> > +
> >     /* Prefer parsed partitions over driver-provided fallback */
> >     ret = parse_mtd_partitions(mtd, types, parser_data);
> >     if (ret == -EPROBE_DEFER)
>
> Thanks,
> Miquèl

Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Miquel Raynal]

>> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
>> > +           mtd->dev = concat->subdev[0]->dev;
>> > +
>> > +           /* Register the platform device */
>> > +           ret = mtd_device_register(mtd, NULL, 0);
>> > +           if (ret)
>> > +                   goto destroy_concat;
>> > +   }
>> > +
>> > +   return 0;
>> > +
>> > +destroy_concat:
>> > +   mtd_concat_destroy(mtd);
>> > +
>> > +   return ret;
>> > +}
>> > +
>> > +late_initcall(mtd_virt_concat_create_join);
>>
>> The current implementation does not support probe deferrals, I believe it should be
>> handled.
>
> I see that the parse_mtd_partitions() API can return -EPROBE_DEFER during
> MTD device registration, but this behavior is specific to the
> parse_qcomsmem_part parser. None of the other parsers appear to support
> probe deferral. As discussed in RFC [1], the virtual concat feature is
> purely a fixed-partition capability, and based on my understanding, the
> fixed-partition parser does not support probe deferral.
> Please let me know if you can think of any other probe deferral scenarios
> that might impact the virtual concat driver.

That's true, but I kind of dislike the late_initcall, I fear it might
break in creative ways.

Thanks,
Miquèl

RE: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Mahapatra, Amit Kumar]

[AMD Official Use Only - AMD Internal Distribution Only]

Hello Miquel,

> >> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
> >> > +           mtd->dev = concat->subdev[0]->dev;
> >> > +
> >> > +           /* Register the platform device */
> >> > +           ret = mtd_device_register(mtd, NULL, 0);
> >> > +           if (ret)
> >> > +                   goto destroy_concat;
> >> > +   }
> >> > +
> >> > +   return 0;
> >> > +
> >> > +destroy_concat:
> >> > +   mtd_concat_destroy(mtd);
> >> > +
> >> > +   return ret;
> >> > +}
> >> > +
> >> > +late_initcall(mtd_virt_concat_create_join);
> >>
> >> The current implementation does not support probe deferrals, I
> >> believe it should be handled.
> >
> > I see that the parse_mtd_partitions() API can return -EPROBE_DEFER
> > during MTD device registration, but this behavior is specific to the
> > parse_qcomsmem_part parser. None of the other parsers appear to
> > support probe deferral. As discussed in RFC [1], the virtual concat
> > feature is purely a fixed-partition capability, and based on my
> > understanding, the fixed-partition parser does not support probe deferral.
> > Please let me know if you can think of any other probe deferral
> > scenarios that might impact the virtual concat driver.
>
> That's true, but I kind of dislike the late_initcall, I fear it might break in creative ways.

I understand, but since we require the partition information to be
available, late_initcall seems to be the most suitable choice among the
initcall levels—if we decide to proceed with using an initcall.
Regarding potential failures, as far as I can tell, the operation would
fail if, at the time of concatenation, one or more of the MTD devices
involved in the concat are not yet available. In such a scenario, we can
issue a kernel warning and exit gracefully. But, However, if you prefer
to move away from using initcalls and have an alternative
implementation approach in mind, please let us know.

Regards,
Amit
>
> Thanks,
> Miquèl

Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Miquel Raynal]

On 13/05/2025 at 14:45:39 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hello Miquel,
>
>> >> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
>> >> > +           mtd->dev = concat->subdev[0]->dev;
>> >> > +
>> >> > +           /* Register the platform device */
>> >> > +           ret = mtd_device_register(mtd, NULL, 0);
>> >> > +           if (ret)
>> >> > +                   goto destroy_concat;
>> >> > +   }
>> >> > +
>> >> > +   return 0;
>> >> > +
>> >> > +destroy_concat:
>> >> > +   mtd_concat_destroy(mtd);
>> >> > +
>> >> > +   return ret;
>> >> > +}
>> >> > +
>> >> > +late_initcall(mtd_virt_concat_create_join);
>> >>
>> >> The current implementation does not support probe deferrals, I
>> >> believe it should be handled.
>> >
>> > I see that the parse_mtd_partitions() API can return -EPROBE_DEFER
>> > during MTD device registration, but this behavior is specific to the
>> > parse_qcomsmem_part parser. None of the other parsers appear to
>> > support probe deferral. As discussed in RFC [1], the virtual concat
>> > feature is purely a fixed-partition capability, and based on my
>> > understanding, the fixed-partition parser does not support probe deferral.
>> > Please let me know if you can think of any other probe deferral
>> > scenarios that might impact the virtual concat driver.
>>
>> That's true, but I kind of dislike the late_initcall, I fear it might break in creative ways.
>
> I understand, but since we require the partition information to be
> available, late_initcall seems to be the most suitable choice among the
> initcall levels—if we decide to proceed with using an initcall.
> Regarding potential failures, as far as I can tell, the operation would
> fail if, at the time of concatenation, one or more of the MTD devices
> involved in the concat are not yet available. In such a scenario, we can
> issue a kernel warning and exit gracefully. But, However, if you prefer
> to move away from using initcalls and have an alternative
> implementation approach in mind, please let us know.

I am sorry but this does not work with modules, and we cannot ignore this
case I believe. More specifically, if a controller probe is deferred
(with EPROBE_DEFER or just prevented because some dependencies are not
yet satisfied), you'll get incorrectly defined mtd devices.

Thanks,
Miquèl

RE: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Mahapatra, Amit Kumar]

[AMD Official Use Only - AMD Internal Distribution Only]

> On 13/05/2025 at 14:45:39 GMT, "Mahapatra, Amit Kumar" <amit.kumar-
> mahapatra@amd.com> wrote:
>
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hello Miquel,
> >
> >> >> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
> >> >> > +           mtd->dev = concat->subdev[0]->dev;
> >> >> > +
> >> >> > +           /* Register the platform device */
> >> >> > +           ret = mtd_device_register(mtd, NULL, 0);
> >> >> > +           if (ret)
> >> >> > +                   goto destroy_concat;
> >> >> > +   }
> >> >> > +
> >> >> > +   return 0;
> >> >> > +
> >> >> > +destroy_concat:
> >> >> > +   mtd_concat_destroy(mtd);
> >> >> > +
> >> >> > +   return ret;
> >> >> > +}
> >> >> > +
> >> >> > +late_initcall(mtd_virt_concat_create_join);
> >> >>
> >> >> The current implementation does not support probe deferrals, I
> >> >> believe it should be handled.
> >> >
> >> > I see that the parse_mtd_partitions() API can return -EPROBE_DEFER
> >> > during MTD device registration, but this behavior is specific to
> >> > the parse_qcomsmem_part parser. None of the other parsers appear to
> >> > support probe deferral. As discussed in RFC [1], the virtual concat
> >> > feature is purely a fixed-partition capability, and based on my
> >> > understanding, the fixed-partition parser does not support probe deferral.
> >> > Please let me know if you can think of any other probe deferral
> >> > scenarios that might impact the virtual concat driver.
> >>
> >> That's true, but I kind of dislike the late_initcall, I fear it might break in creative
> ways.
> >
> > I understand, but since we require the partition information to be
> > available, late_initcall seems to be the most suitable choice among
> > the initcall levels—if we decide to proceed with using an initcall.
> > Regarding potential failures, as far as I can tell, the operation
> > would fail if, at the time of concatenation, one or more of the MTD
> > devices involved in the concat are not yet available. In such a
> > scenario, we can issue a kernel warning and exit gracefully. But,
> > However, if you prefer to move away from using initcalls and have an
> > alternative implementation approach in mind, please let us know.
>
> I am sorry but this does not work with modules, and we cannot ignore this case I
> believe. More specifically, if a controller probe is deferred (with EPROBE_DEFER
> or just prevented because some dependencies are not yet satisfied), you'll get
> incorrectly defined mtd devices.

Ok, an alternative solution could be to remove the initcall registration
and instead invoke mtd_virt_concat_create_join()—which was previously
registered as a late_initcall—directly from mtd_device_parse_register().
I believe this approach would address both of your concerns regarding
module support and probe deferral. Additionally, we could consider
moving the entire code from mtd_virt_concat.c into mtdconcat.c.
Please let us know your take on this.

Regards,
Amit
>
> Thanks,
> Miquèl

Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Miquel Raynal]

On 21/05/2025 at 06:13:32 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
>
>> On 13/05/2025 at 14:45:39 GMT, "Mahapatra, Amit Kumar" <amit.kumar-
>> mahapatra@amd.com> wrote:
>>
>> > [AMD Official Use Only - AMD Internal Distribution Only]
>> >
>> > Hello Miquel,
>> >
>> >> >> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
>> >> >> > +           mtd->dev = concat->subdev[0]->dev;
>> >> >> > +
>> >> >> > +           /* Register the platform device */
>> >> >> > +           ret = mtd_device_register(mtd, NULL, 0);
>> >> >> > +           if (ret)
>> >> >> > +                   goto destroy_concat;
>> >> >> > +   }
>> >> >> > +
>> >> >> > +   return 0;
>> >> >> > +
>> >> >> > +destroy_concat:
>> >> >> > +   mtd_concat_destroy(mtd);
>> >> >> > +
>> >> >> > +   return ret;
>> >> >> > +}
>> >> >> > +
>> >> >> > +late_initcall(mtd_virt_concat_create_join);
>> >> >>
>> >> >> The current implementation does not support probe deferrals, I
>> >> >> believe it should be handled.
>> >> >
>> >> > I see that the parse_mtd_partitions() API can return -EPROBE_DEFER
>> >> > during MTD device registration, but this behavior is specific to
>> >> > the parse_qcomsmem_part parser. None of the other parsers appear to
>> >> > support probe deferral. As discussed in RFC [1], the virtual concat
>> >> > feature is purely a fixed-partition capability, and based on my
>> >> > understanding, the fixed-partition parser does not support probe deferral.
>> >> > Please let me know if you can think of any other probe deferral
>> >> > scenarios that might impact the virtual concat driver.
>> >>
>> >> That's true, but I kind of dislike the late_initcall, I fear it might break in creative
>> ways.
>> >
>> > I understand, but since we require the partition information to be
>> > available, late_initcall seems to be the most suitable choice among
>> > the initcall levels—if we decide to proceed with using an initcall.
>> > Regarding potential failures, as far as I can tell, the operation
>> > would fail if, at the time of concatenation, one or more of the MTD
>> > devices involved in the concat are not yet available. In such a
>> > scenario, we can issue a kernel warning and exit gracefully. But,
>> > However, if you prefer to move away from using initcalls and have an
>> > alternative implementation approach in mind, please let us know.
>>
>> I am sorry but this does not work with modules, and we cannot ignore this case I
>> believe. More specifically, if a controller probe is deferred (with EPROBE_DEFER
>> or just prevented because some dependencies are not yet satisfied), you'll get
>> incorrectly defined mtd devices.
>
> Ok, an alternative solution could be to remove the initcall registration
> and instead invoke mtd_virt_concat_create_join()—which was previously
> registered as a late_initcall—directly from mtd_device_parse_register().
> I believe this approach would address both of your concerns regarding
> module support and probe deferral. Additionally, we could consider
> moving the entire code from mtd_virt_concat.c into mtdconcat.c.
> Please let us know your take on this.

What would this bring?

Maybe we should trigger some kind of notifier after registering an mtd
device and in there attempt to gather all mtd devices required for the
concatenation. Can you please propose something like that?

Thanks,
Miquèl

RE: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Mahapatra, Amit Kumar]

[AMD Official Use Only - AMD Internal Distribution Only]

> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> >> On 13/05/2025 at 14:45:39 GMT, "Mahapatra, Amit Kumar" <amit.kumar-
> >> mahapatra@amd.com> wrote:
> >>
> >> > [AMD Official Use Only - AMD Internal Distribution Only]
> >> >
> >> > Hello Miquel,
> >> >
> >> >> >> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
> >> >> >> > +           mtd->dev = concat->subdev[0]->dev;
> >> >> >> > +
> >> >> >> > +           /* Register the platform device */
> >> >> >> > +           ret = mtd_device_register(mtd, NULL, 0);
> >> >> >> > +           if (ret)
> >> >> >> > +                   goto destroy_concat;
> >> >> >> > +   }
> >> >> >> > +
> >> >> >> > +   return 0;
> >> >> >> > +
> >> >> >> > +destroy_concat:
> >> >> >> > +   mtd_concat_destroy(mtd);
> >> >> >> > +
> >> >> >> > +   return ret;
> >> >> >> > +}
> >> >> >> > +
> >> >> >> > +late_initcall(mtd_virt_concat_create_join);
> >> >> >>
> >> >> >> The current implementation does not support probe deferrals, I
> >> >> >> believe it should be handled.
> >> >> >
> >> >> > I see that the parse_mtd_partitions() API can return
> >> >> > -EPROBE_DEFER during MTD device registration, but this behavior
> >> >> > is specific to the parse_qcomsmem_part parser. None of the other
> >> >> > parsers appear to support probe deferral. As discussed in RFC
> >> >> > [1], the virtual concat feature is purely a fixed-partition
> >> >> > capability, and based on my understanding, the fixed-partition parser does
> not support probe deferral.
> >> >> > Please let me know if you can think of any other probe deferral
> >> >> > scenarios that might impact the virtual concat driver.
> >> >>
> >> >> That's true, but I kind of dislike the late_initcall, I fear it
> >> >> might break in creative
> >> ways.
> >> >
> >> > I understand, but since we require the partition information to be
> >> > available, late_initcall seems to be the most suitable choice among
> >> > the initcall levels—if we decide to proceed with using an initcall.
> >> > Regarding potential failures, as far as I can tell, the operation
> >> > would fail if, at the time of concatenation, one or more of the MTD
> >> > devices involved in the concat are not yet available. In such a
> >> > scenario, we can issue a kernel warning and exit gracefully. But,
> >> > However, if you prefer to move away from using initcalls and have
> >> > an alternative implementation approach in mind, please let us know.
> >>
> >> I am sorry but this does not work with modules, and we cannot ignore
> >> this case I believe. More specifically, if a controller probe is
> >> deferred (with EPROBE_DEFER or just prevented because some
> >> dependencies are not yet satisfied), you'll get incorrectly defined mtd devices.
> >
> > Ok, an alternative solution could be to remove the initcall
> > registration and instead invoke mtd_virt_concat_create_join()—which
> > was previously registered as a late_initcall—directly from
> mtd_device_parse_register().
> > I believe this approach would address both of your concerns regarding
> > module support and probe deferral. Additionally, we could consider
> > moving the entire code from mtd_virt_concat.c into mtdconcat.c.
> > Please let us know your take on this.
>
> What would this bring?
>
> Maybe we should trigger some kind of notifier after registering an mtd device and in
> there attempt to gather all mtd devices required for the concatenation. Can you
> please propose something like that?

In the current patch, during MTD registration, if a device is
part of a concatenated (concat) device, it is not registered individually.
Instead, its information is stored in a concat-specific data structure, as
it is not meant to be exposed as a standalone MTD device. As per my
earlier proposal, once all individual MTD devices are registered,
mtd_virt_concat_create_join() is called from
mtd_device_parse_register() to scan this data structure and create the
corresponding concat devices. Just to confirm, are you suggesting that
mtd_virt_concat_create_join() should be triggered through a notifier
instead? At the point when all individual MTD devices are registered,
we already have the complete information required for concatenation.
So, rather than relying on a listener notification, we cac directly call the
API. Please let me know if I am missing anything here.

Regards,
Amit
>
> Thanks,
> Miquèl

Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Miquel Raynal]

On 26/05/2025 at 14:27:37 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
>
>> > [AMD Official Use Only - AMD Internal Distribution Only]
>> >
>> >> On 13/05/2025 at 14:45:39 GMT, "Mahapatra, Amit Kumar" <amit.kumar-
>> >> mahapatra@amd.com> wrote:
>> >>
>> >> > [AMD Official Use Only - AMD Internal Distribution Only]
>> >> >
>> >> > Hello Miquel,
>> >> >
>> >> >> >> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
>> >> >> >> > +           mtd->dev = concat->subdev[0]->dev;
>> >> >> >> > +
>> >> >> >> > +           /* Register the platform device */
>> >> >> >> > +           ret = mtd_device_register(mtd, NULL, 0);
>> >> >> >> > +           if (ret)
>> >> >> >> > +                   goto destroy_concat;
>> >> >> >> > +   }
>> >> >> >> > +
>> >> >> >> > +   return 0;
>> >> >> >> > +
>> >> >> >> > +destroy_concat:
>> >> >> >> > +   mtd_concat_destroy(mtd);
>> >> >> >> > +
>> >> >> >> > +   return ret;
>> >> >> >> > +}
>> >> >> >> > +
>> >> >> >> > +late_initcall(mtd_virt_concat_create_join);
>> >> >> >>
>> >> >> >> The current implementation does not support probe deferrals, I
>> >> >> >> believe it should be handled.
>> >> >> >
>> >> >> > I see that the parse_mtd_partitions() API can return
>> >> >> > -EPROBE_DEFER during MTD device registration, but this behavior
>> >> >> > is specific to the parse_qcomsmem_part parser. None of the other
>> >> >> > parsers appear to support probe deferral. As discussed in RFC
>> >> >> > [1], the virtual concat feature is purely a fixed-partition
>> >> >> > capability, and based on my understanding, the fixed-partition parser does
>> not support probe deferral.
>> >> >> > Please let me know if you can think of any other probe deferral
>> >> >> > scenarios that might impact the virtual concat driver.
>> >> >>
>> >> >> That's true, but I kind of dislike the late_initcall, I fear it
>> >> >> might break in creative
>> >> ways.
>> >> >
>> >> > I understand, but since we require the partition information to be
>> >> > available, late_initcall seems to be the most suitable choice among
>> >> > the initcall levels—if we decide to proceed with using an initcall.
>> >> > Regarding potential failures, as far as I can tell, the operation
>> >> > would fail if, at the time of concatenation, one or more of the MTD
>> >> > devices involved in the concat are not yet available. In such a
>> >> > scenario, we can issue a kernel warning and exit gracefully. But,
>> >> > However, if you prefer to move away from using initcalls and have
>> >> > an alternative implementation approach in mind, please let us know.
>> >>
>> >> I am sorry but this does not work with modules, and we cannot ignore
>> >> this case I believe. More specifically, if a controller probe is
>> >> deferred (with EPROBE_DEFER or just prevented because some
>> >> dependencies are not yet satisfied), you'll get incorrectly defined mtd devices.
>> >
>> > Ok, an alternative solution could be to remove the initcall
>> > registration and instead invoke mtd_virt_concat_create_join()—which
>> > was previously registered as a late_initcall—directly from
>> mtd_device_parse_register().
>> > I believe this approach would address both of your concerns regarding
>> > module support and probe deferral. Additionally, we could consider
>> > moving the entire code from mtd_virt_concat.c into mtdconcat.c.
>> > Please let us know your take on this.
>>
>> What would this bring?
>>
>> Maybe we should trigger some kind of notifier after registering an mtd device and in
>> there attempt to gather all mtd devices required for the concatenation. Can you
>> please propose something like that?
>
> In the current patch, during MTD registration, if a device is
> part of a concatenated (concat) device, it is not registered individually.
> Instead, its information is stored in a concat-specific data structure, as
> it is not meant to be exposed as a standalone MTD device. As per my
> earlier proposal, once all individual MTD devices are registered,
> mtd_virt_concat_create_join() is called from
> mtd_device_parse_register() to scan this data structure and create the
> corresponding concat devices. Just to confirm, are you suggesting that
> mtd_virt_concat_create_join() should be triggered through a notifier
> instead? At the point when all individual MTD devices are registered,
> we already have the complete information required for concatenation.
> So, rather than relying on a listener notification, we cac directly call the
> API. Please let me know if I am missing anything here.

This approach does not stand because, afair, it relies on a single
late_initcall() which is too early. We want concatenation to work
regardless of the Kconfig selection =y or =m.

Thanks,
Miquèl

RE: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Mahapatra, Amit Kumar]

[AMD Official Use Only - AMD Internal Distribution Only]

> On 26/05/2025 at 14:27:37 GMT, "Mahapatra, Amit Kumar" <amit.kumar-
> mahapatra@amd.com> wrote:
>
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> >> > [AMD Official Use Only - AMD Internal Distribution Only]
> >> >
> >> >> On 13/05/2025 at 14:45:39 GMT, "Mahapatra, Amit Kumar"
> >> >> <amit.kumar- mahapatra@amd.com> wrote:
> >> >>
> >> >> > [AMD Official Use Only - AMD Internal Distribution Only]
> >> >> >
> >> >> > Hello Miquel,
> >> >> >
> >> >> >> >> > +           mtd->dev.parent = concat->subdev[0]->dev.parent;
> >> >> >> >> > +           mtd->dev = concat->subdev[0]->dev;
> >> >> >> >> > +
> >> >> >> >> > +           /* Register the platform device */
> >> >> >> >> > +           ret = mtd_device_register(mtd, NULL, 0);
> >> >> >> >> > +           if (ret)
> >> >> >> >> > +                   goto destroy_concat;
> >> >> >> >> > +   }
> >> >> >> >> > +
> >> >> >> >> > +   return 0;
> >> >> >> >> > +
> >> >> >> >> > +destroy_concat:
> >> >> >> >> > +   mtd_concat_destroy(mtd);
> >> >> >> >> > +
> >> >> >> >> > +   return ret;
> >> >> >> >> > +}
> >> >> >> >> > +
> >> >> >> >> > +late_initcall(mtd_virt_concat_create_join);
> >> >> >> >>
> >> >> >> >> The current implementation does not support probe deferrals,
> >> >> >> >> I believe it should be handled.
> >> >> >> >
> >> >> >> > I see that the parse_mtd_partitions() API can return
> >> >> >> > -EPROBE_DEFER during MTD device registration, but this
> >> >> >> > behavior is specific to the parse_qcomsmem_part parser. None
> >> >> >> > of the other parsers appear to support probe deferral. As
> >> >> >> > discussed in RFC [1], the virtual concat feature is purely a
> >> >> >> > fixed-partition capability, and based on my understanding,
> >> >> >> > the fixed-partition parser does
> >> not support probe deferral.
> >> >> >> > Please let me know if you can think of any other probe
> >> >> >> > deferral scenarios that might impact the virtual concat driver.
> >> >> >>
> >> >> >> That's true, but I kind of dislike the late_initcall, I fear it
> >> >> >> might break in creative
> >> >> ways.
> >> >> >
> >> >> > I understand, but since we require the partition information to
> >> >> > be available, late_initcall seems to be the most suitable choice
> >> >> > among the initcall levels—if we decide to proceed with using an initcall.
> >> >> > Regarding potential failures, as far as I can tell, the
> >> >> > operation would fail if, at the time of concatenation, one or
> >> >> > more of the MTD devices involved in the concat are not yet
> >> >> > available. In such a scenario, we can issue a kernel warning and
> >> >> > exit gracefully. But, However, if you prefer to move away from
> >> >> > using initcalls and have an alternative implementation approach in mind,
> please let us know.
> >> >>
> >> >> I am sorry but this does not work with modules, and we cannot
> >> >> ignore this case I believe. More specifically, if a controller
> >> >> probe is deferred (with EPROBE_DEFER or just prevented because
> >> >> some dependencies are not yet satisfied), you'll get incorrectly defined mtd
> devices.
> >> >
> >> > Ok, an alternative solution could be to remove the initcall
> >> > registration and instead invoke mtd_virt_concat_create_join()—which
> >> > was previously registered as a late_initcall—directly from
> >> mtd_device_parse_register().
> >> > I believe this approach would address both of your concerns
> >> > regarding module support and probe deferral. Additionally, we could
> >> > consider moving the entire code from mtd_virt_concat.c into mtdconcat.c.
> >> > Please let us know your take on this.
> >>
> >> What would this bring?
> >>
> >> Maybe we should trigger some kind of notifier after registering an
> >> mtd device and in there attempt to gather all mtd devices required
> >> for the concatenation. Can you please propose something like that?
> >
> > In the current patch, during MTD registration, if a device is part of
> > a concatenated (concat) device, it is not registered individually.
> > Instead, its information is stored in a concat-specific data
> > structure, as it is not meant to be exposed as a standalone MTD
> > device. As per my earlier proposal, once all individual MTD devices
> > are registered,
> > mtd_virt_concat_create_join() is called from
> > mtd_device_parse_register() to scan this data structure and create the
> > corresponding concat devices. Just to confirm, are you suggesting that
> > mtd_virt_concat_create_join() should be triggered through a notifier
> > instead? At the point when all individual MTD devices are registered,
> > we already have the complete information required for concatenation.
> > So, rather than relying on a listener notification, we cac directly
> > call the API. Please let me know if I am missing anything here.
>
> This approach does not stand because, afair, it relies on a single
> late_initcall() which is too early. We want concatenation to work regardless of the
> Kconfig selection =y or =m.

In this approach, late_initcall() has been removed. Instead, we explicitly
call the API at [1] to create the concat device, as by this stage all
partitions have been parsed, individual MTD devices registered, and the
necessary information for creating the concat device has been gathered.

[1] https://github.com/torvalds/linux/blob/0ff41df1cb268fc69e703a08a57ee14ae967d0ca/drivers/mtd/mtdcore.c#L1089

Regards,
Amit
>
> Thanks,
> Miquèl

Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Miquel Raynal]

> In this approach, late_initcall() has been removed. Instead, we explicitly
> call the API at [1] to create the concat device, as by this stage all
> partitions have been parsed, individual MTD devices registered, and the
> necessary information for creating the concat device has been
> gathered.

I believe I made comments about the new approach but please resend if
you think I'm wrong, I will have a second look.

Miquèl