mtd: read partitions compatible prop for "ofpart" type

PATCH In review Rafał Miłecki 5 participants 13 messages Jan 30, 2018 – Mar 27, 2018 1 message not in the archive Memory technology devices (MTD)

lore

Share & tools

lore

Rafał Miłecki

author

2018-01-30 20:10 UTC #0 lore raw

v9 cover mtd: read partitions compatible prop for "ofpart" type

From: Rafał Miłecki <[email protected]>

This patchset provides a proper support for flash device DT node with "partitions" subnode using "compatible" property. It's already documented in the: Documentation/devicetree/bindings/mtd/partition.txt

We believed a previous version 8 was ready to go, but soon after landing in the linux-next we got a regression report from Peter.

This version takes a safe path by:
1) Respecting parsers order as specified in the default/driver-provided list.
2) Looking at "compatible" property only when "ofpart" type gets speciied.

I double-checked the code and cannot think of any regression this could cause. I also hope this design (roughly discussed with Boris) can be acceptable for the mtd subsystem.

This is of course 4.17 material at best.

Brian Norris (1):
mtd: partitions: add of_match_table parser matching

Rafał Miłecki (3):
mtd: partitions: add special treating for the "ofpart" parser type
mtd: rename "ofpart" parser to "fixed-partitions" as it fits it better
mtd: ofpart: add of_match_table with "fixed-partitions"

 drivers/mtd/mtdpart.c          | 116 +++++++++++++++++++++++++++++++++++++----
 drivers/mtd/ofpart.c           |  18 +++++--
 include/linux/mtd/partitions.h |   1 +
 3 files changed, 121 insertions(+), 14 deletions(-)

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180130201059.4424-1-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Rafał Miłecki

author

2018-01-30 20:10 UTC #2 lore raw

PATCH v9 1/4 mtd: partitions: add special treating for the "ofpart" parser type

From: Rafał Miłecki <[email protected]>

In order to properly support compatibility strings as described in the bindings/mtd/partition.txt "ofpart" type should be treated as an indication for looking into OF. MTD should check "compatible" property and search for a matching parser rather than blindly trying a one for the "fixed-partitions".

This also means that existing parser should get renamed into a more meaningful one.

Signed-off-by Rafał Miłecki <[email protected]>

---
V9: First appearance of this commit

---
 drivers/mtd/mtdpart.c | 61 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 9 deletions(-)

drivers/mtd/mtdpart.c +52 -9

index 76cd21d1171b..d40ec525ceff 100644
@@ -860,6 +860,38 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser,
 	return ret;
 }
 
+static int mtd_part_of_parse(struct mtd_info *master,
+			     struct mtd_partitions *pparts)
+{
+	struct mtd_part_parser *parser;
+	const char *fixed = "ofpart";
+	int ret, err = 0;
+
+	/*
+	 * TODO: Check "compatible" DT property and look for a matching parser.
+	 */
+
+	/*
+	 * For backward compatibility we have to try the "ofpart"
+	 * parser. It supports old DT format with partitions specified as a
+	 * direct subnodes of a flash device DT node without any compatibility
+	 * specified we could match.
+	 */
+	parser = mtd_part_parser_get(fixed);
+	if (!parser && !request_module("%s", fixed))
+		parser = mtd_part_parser_get(fixed);
+	if (parser) {
+		ret = mtd_part_do_parse(parser, master, pparts, NULL);
+		if (ret > 0)
+			return ret;
+		mtd_part_parser_put(parser);
+		if (ret < 0 && !err)
+			err = ret;
+	}
+
+	return err;
+}
+
 /**
  * parse_mtd_partitions - parse MTD partitions
  * @master: the master partition (describes whole MTD device)

@@ -892,19 +924,30 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		types = default_mtd_part_types;
 
 	for ( ; *types; types++) {
-		pr_debug("%s: parsing partitions %s\n", master->name, *types);
-		parser = mtd_part_parser_get(*types);
-		if (!parser && !request_module("%s", *types))
+		/*
+		 * ofpart is a special type that means OF partitioning info
+		 * should be used. It requires a bit different logic so it is
+		 * handled in a separated function.
+		 */
+		if (!strcmp(*types, "ofpart")) {
+			ret = mtd_part_of_parse(master, pparts);
+		} else {
+			pr_debug("%s: parsing partitions %s\n", master->name,
+				 *types);
 			parser = mtd_part_parser_get(*types);
-		pr_debug("%s: got parser %s\n", master->name,
-			 parser ? parser->name : NULL);
-		if (!parser)
-			continue;
-		ret = mtd_part_do_parse(parser, master, pparts, data);
+			if (!parser && !request_module("%s", *types))
+				parser = mtd_part_parser_get(*types);
+			pr_debug("%s: got parser %s\n", master->name,
+				parser ? parser->name : NULL);
+			if (!parser)
+				continue;
+			ret = mtd_part_do_parse(parser, master, pparts, data);
+			if (ret <= 0)
+				mtd_part_parser_put(parser);
+		}
 		/* Found partitions! */
 		if (ret > 0)
 			return 0;
-		mtd_part_parser_put(parser);
 		/*
 		 * Stash the first error we see; only report it if no parser
 		 * succeeds

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafał Miłecki

author

2018-01-30 20:10 UTC #3 lore raw

PATCH v9 2/4 mtd: rename "ofpart" parser to "fixed-partitions" as it fits it better

In reply to a message not in the archive

From: Rafał Miłecki <[email protected]>

Type "ofpart" means that OF should be used to get partitioning info and this driver supports "fixed-partitions" binding only. Renaming it should lead to less confusion especially when parsers for new compatibility strings start to appear.

Signed-off-by Rafał Miłecki <[email protected]>

---
V9: First appearance of this commit

---
 drivers/mtd/mtdpart.c |  4 ++--
 drivers/mtd/ofpart.c  | 11 ++++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

drivers/mtd/mtdpart.c +2 -2

index d40ec525ceff..a29bbcf79691 100644
@@ -864,7 +864,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
 			     struct mtd_partitions *pparts)
 {
 	struct mtd_part_parser *parser;
-	const char *fixed = "ofpart";
+	const char *fixed = "fixed-partitions";
 	int ret, err = 0;
 
 	/*

@@ -872,7 +872,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
 	 */
 
 	/*
-	 * For backward compatibility we have to try the "ofpart"
+	 * For backward compatibility we have to try the "fixed-partitions"
 	 * parser. It supports old DT format with partitions specified as a
 	 * direct subnodes of a flash device DT node without any compatibility
 	 * specified we could match.

drivers/mtd/ofpart.c +6 -5

index 6bdf4e525677..9f497315e65d 100644
@@ -25,9 +25,9 @@ static bool node_has_compatible(struct device_node *pp)
 	return of_get_property(pp, "compatible", NULL);
 }
 
-static int parse_ofpart_partitions(struct mtd_info *master,
-				   const struct mtd_partition **pparts,
-				   struct mtd_part_parser_data *data)
+static int parse_fixed_partitions(struct mtd_info *master,
+				  const struct mtd_partition **pparts,
+				  struct mtd_part_parser_data *data)
 {
 	struct mtd_partition *parts;
 	struct device_node *mtd_node;

@@ -141,8 +141,8 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 }
 
 static struct mtd_part_parser ofpart_parser = {
-	.parse_fn = parse_ofpart_partitions,
-	.name = "ofpart",
+	.parse_fn = parse_fixed_partitions,
+	.name = "fixed-partitions",
 };
 
 static int parse_ofoldpart_partitions(struct mtd_info *master,

@@ -229,4 +229,5 @@ MODULE_AUTHOR("Vitaly Wool, David Gibson");
  * with the same name. Since we provide the ofoldpart parser, we should have
  * the corresponding alias.
  */
+MODULE_ALIAS("fixed-partitions");
 MODULE_ALIAS("ofoldpart");

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafał Miłecki

author

2018-01-30 20:10 UTC #4 lore raw

PATCH v9 3/4 mtd: partitions: add of_match_table parser matching

In reply to a message not in the archive

From: Brian Norris <[email protected]>

Partition parsers can now provide an of_match_table to enable flash<-->parser matching via device tree as documented in the mtd/partition.txt.

It works by looking for a matching parser for every string in the "compatibility" property (starting with the most specific one). Please note that driver-specified parsers still take a precedence. It's assumed that driver providing a parser type has a good reason for that (e.g. having platform data with device-specific info). Also doing otherwise could break existing setups. The same applies to using default parsers (including "cmdlinepart") as some overwrite DT data with cmdline argument.

This support is currently limited to built-in parsers as it uses request_module() and friends. This should be sufficient for most cases though as compiling parsers as modules isn't a common choice.

Signed-off-by Brian Norris <[email protected]>

Signed-off-by Rafał Miłecki <[email protected]>

---
This is based on Brian's patches:
[RFC PATCH 4/7] mtd: add of_match_mtd_parser() and of_mtd_match_mtd_parser() helpers [RFC PATCH 6/7] RFC: mtd: partitions: enable of_match_table matching

V1: Put helpers in mtdpart.c instead of drivers/of/of_mtd.c
Merge helpers into a single of_mtd_match_mtd_parser
V3: Add a simple comment to note we will need the best match in the future
V4: Rework new functions to pick parser with the best match
Move new code in parse_mtd_partitions up so it has precedence over flash driver defaults and MTD defaults
V5: Rework matching code to start checking with the most specific string in the
"compatibility" property.
V6: Initialize "ret" variable in mtd_part_get_parser_by_cp to NULL
V7: Rename function "mtd_part_get_parser_by_cp" to use "_compatible_"
Rename "cp" variable to "compat"
Drop unneeded if (np) check from the parse_mtd_partitions
V8: Move new OF code in parse_mtd_partitions() lower so driver-specified parsers
take a precedence. Update commit message to make that clear.
V9: Update to modify mtd_part_of_parse() instead of parse_mtd_partitions()

---
 drivers/mtd/mtdpart.c          | 61 +++++++++++++++++++++++++++++++++++++++---
 include/linux/mtd/partitions.h |  1 +
 2 files changed, 59 insertions(+), 3 deletions(-)

drivers/mtd/mtdpart.c +58 -3

index a29bbcf79691..cdb1f496ab95 100644
@@ -30,6 +30,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/err.h>
+#include <linux/of.h>
 
 #include "mtdcore.h"

@@ -860,16 +861,70 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser,
 	return ret;
 }
 
+/**
+ * mtd_part_get_compatible_parser - find MTD parser by a compatible string
+ *
+ * @compat: compatible string describing partitions in a device tree
+ *
+ * MTD parsers can specify supported partitions by providing a table of
+ * compatibility strings. This function finds a parser that advertises support
+ * for a passed value of "compatible".
+ */
+static struct mtd_part_parser *mtd_part_get_compatible_parser(const char *compat)
+{
+	struct mtd_part_parser *p, *ret = NULL;
+
+	spin_lock(&part_parser_lock);
+
+	list_for_each_entry(p, &part_parsers, list) {
+		const struct of_device_id *matches;
+
+		matches = p->of_match_table;
+		if (!matches)
+			continue;
+
+		for (; matches->compatible[0]; matches++) {
+			if (!strcmp(matches->compatible, compat) &&
+			    try_module_get(p->owner)) {
+				ret = p;
+				break;
+			}
+		}
+
+		if (ret)
+			break;
+	}
+
+	spin_unlock(&part_parser_lock);
+
+	return ret;
+}
+
 static int mtd_part_of_parse(struct mtd_info *master,
 			     struct mtd_partitions *pparts)
 {
 	struct mtd_part_parser *parser;
+	struct device_node *np;
+	struct property *prop;
+	const char *compat;
 	const char *fixed = "fixed-partitions";
 	int ret, err = 0;
 
-	/*
-	 * TODO: Check "compatible" DT property and look for a matching parser.
-	 */
+	np = of_get_child_by_name(mtd_get_of_node(master), "partitions");
+	of_property_for_each_string(np, "compatible", prop, compat) {
+		parser = mtd_part_get_compatible_parser(compat);
+		if (!parser)
+			continue;
+		ret = mtd_part_do_parse(parser, master, pparts, NULL);
+		if (ret > 0) {
+			of_node_put(np);
+			return ret;
+		}
+		mtd_part_parser_put(parser);
+		if (ret < 0 && !err)
+			err = ret;
+	}
+	of_node_put(np);
 
 	/*
 	 * For backward compatibility we have to try the "fixed-partitions"

include/linux/mtd/partitions.h +1 -0

index c4beb70dacbd..11cb0c50cd84 100644
@@ -77,6 +77,7 @@ struct mtd_part_parser {
 	struct list_head list;
 	struct module *owner;
 	const char *name;
+	const struct of_device_id *of_match_table;
 	int (*parse_fn)(struct mtd_info *, const struct mtd_partition **,
 			struct mtd_part_parser_data *);
 	void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafał Miłecki

author

2018-01-30 20:10 UTC #5 lore raw

PATCH v9 4/4 mtd: ofpart: add of_match_table with "fixed-partitions"

In reply to a message not in the archive

From: Rafał Miłecki <[email protected]>

This allows using this parser with any flash driver that takes care of setting of_node (using mtd_set_of_node helper) correctly. Up to now support for "fixed-partitions" DT compatibility string was working only with flash drivers that were specifying "ofpart" (manually or by letting mtd use the default set of parsers).

This matches existing bindings documentation.

Signed-off-by Rafał Miłecki <[email protected]>

Reviewed-by Brian Norris <[email protected]>

Tested-by Brian Norris <[email protected]>

---
 drivers/mtd/ofpart.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/mtd/ofpart.c +7 -0

index 9f497315e65d..615f8c173162 100644
@@ -140,9 +140,16 @@ static int parse_fixed_partitions(struct mtd_info *master,
 	return ret;
 }
 
+static const struct of_device_id parse_ofpart_match_table[] = {
+	{ .compatible = "fixed-partitions" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, parse_ofpart_match_table);
+
 static struct mtd_part_parser ofpart_parser = {
 	.parse_fn = parse_fixed_partitions,
 	.name = "fixed-partitions",
+	.of_match_table = parse_ofpart_match_table,
 };
 
 static int parse_ofoldpart_partitions(struct mtd_info *master,

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Rosin

Tested-by

2018-02-01 14:32 UTC #6 lore raw

In reply to a message not in the archive

Quoted 8 lines · Rafał Miłecki

From: Rafał Miłecki <[email protected]>
This patchset provides a proper support for flash device DT node with "partitions" subnode using "compatible" property. It's already documented in the: Documentation/devicetree/bindings/mtd/partition.txt
We believed a previous version 8 was ready to go, but soon after landing in the linux-next we got a regression report from Peter.

I retested these four patches on-top of 4.15, and results are good.

With a faulty partition layout in the dtb, the old command-line layout wins and things keep working, and with a good partition layout in the dtb I can remove a bunch of command-line clutter.

Tested-by Peter Rosin <[email protected]>

No testing of the individual patches though, just the end result...

Cheers,
peda

Quoted 20 lines

This version takes a safe path by:
1) Respecting parsers order as specified in the default/driver-provided list.
2) Looking at "compatible" property only when "ofpart" type gets speciied.
I double-checked the code and cannot think of any regression this could cause. I also hope this design (roughly discussed with Boris) can be acceptable for the mtd subsystem.
This is of course 4.17 material at best.
Brian Norris (1):
mtd: partitions: add of_match_table parser matching
Rafał Miłecki (3):
mtd: partitions: add special treating for the "ofpart" parser type
mtd: rename "ofpart" parser to "fixed-partitions" as it fits it better
mtd: ofpart: add of_match_table with "fixed-partitions"

Quoted 5 lines

 drivers/mtd/mtdpart.c          | 116 +++++++++++++++++++++++++++++++++++++----
 drivers/mtd/ofpart.c           |  18 +++++--
 include/linux/mtd/partitions.h |   1 +
 3 files changed, 121 insertions(+), 14 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Richard Weinberger

maintainer

2018-03-11 22:33 UTC #7 lore raw

In reply to Rafał Miłecki

Rafał,

Am Dienstag, 30. Januar 2018, 21:10:55 CET schrieb Rafał Miłecki:

Quoted 29 lines

From: Rafał Miłecki <[email protected]>
This patchset provides a proper support for flash device DT node with "partitions" subnode using "compatible" property. It's already documented in the: Documentation/devicetree/bindings/mtd/partition.txt
We believed a previous version 8 was ready to go, but soon after landing in the linux-next we got a regression report from Peter.
This version takes a safe path by:
1) Respecting parsers order as specified in the default/driver-provided list.
2) Looking at "compatible" property only when "ofpart" type gets speciied.
I double-checked the code and cannot think of any regression this could cause. I also hope this design (roughly discussed with Boris) can be acceptable for the mtd subsystem.
This is of course 4.17 material at best.
Brian Norris (1):
mtd: partitions: add of_match_table parser matching
Rafał Miłecki (3):
mtd: partitions: add special treating for the "ofpart" parser type
mtd: rename "ofpart" parser to "fixed-partitions" as it fits it better
mtd: ofpart: add of_match_table with "fixed-partitions"

Quoted 5 lines

 drivers/mtd/mtdpart.c          | 116
+++++++++++++++++++++++++++++++++++++---- drivers/mtd/ofpart.c           | 
18 +++++--
 include/linux/mtd/partitions.h |   1 +
 3 files changed, 121 insertions(+), 14 deletions(-)

As discussed on IRC, 1/4 confused me. It should be squashed into 3/4 to make sense.
Other than that I think we can merge this patch series.
Not that I'm a super fan of it, but I can see the use-case but don't have an idea how to solve the problem in a less hacky way. ;-\

Just to be sure, DT guys are also happy with it, right?

Thanks,
//richard

Rafał Miłecki

2018-03-14 12:10 UTC #8 lore raw

v10 cover mtd: read partitions compatible prop for "ofpart" type

In reply to Rafał Miłecki

From: Rafał Miłecki <[email protected]>

We believed that version 7 was ready to go, but soon after landing in the linux-next we got a regression report from Peter. Later versions takes a safe path by:
1) Respecting parsers order as specified in the default/driver-provided list.
2) Looking at "compatible" property only when "ofpart" type gets speciied.

Version 9 was successfully tested by Peter and version 10 just squashes two commits into a one.

I double-checked the code and cannot think of any regression this could cause. I also hope this design (roughly discussed with Boris) can be acceptable for the mtd subsystem.

The most important patch from this patchset (1/3) was reviewed by Rob in the
[PATCH V6 1/2] mtd: partitions: add of_match_table parser matching
https://patchwork.ozlabs.org/comment/1831551/

If possible this is a 4.17 material.

Rafał Miłecki (3):
mtd: partitions: add of_match_table parser matching for the "ofpart"
type
mtd: rename "ofpart" parser to "fixed-partitions" as it fits it better
mtd: ofpart: add of_match_table with "fixed-partitions"

 drivers/mtd/mtdpart.c          | 116 +++++++++++++++++++++++++++++++++++++----
 drivers/mtd/ofpart.c           |  18 +++++--
 include/linux/mtd/partitions.h |   1 +
 3 files changed, 121 insertions(+), 14 deletions(-)

Signature · 6 lines

2.11.0


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

Rafał Miłecki

2018-03-14 12:10 UTC #9 lore raw

PATCH v10 1/3 mtd: partitions: add of_match_table parser matching for the "ofpart" type

From: Rafał Miłecki <[email protected]>

It also means that existing "fixed-partitions" parser should get renamed to use a more meaningful name.

This commit achievies that aim by introducing a new mtd_part_of_parse(). It works by looking for a matching parser for every string in the "compatibility" property (starting with the most specific one).

Please note that driver-specified parsers still take a precedence. It's assumed that driver providing a parser type has a good reason for that (e.g. having platform data with device-specific info). Also doing otherwise could break existing setups. The same applies to using default parsers (including "cmdlinepart") as some overwrite DT data with cmdline argument.

Partition parsers can now provide an of_match_table to enable flash<-->parser matching via device tree as documented in the mtd/partition.txt.

This support is currently limited to built-in parsers as it uses request_module() and friends. This should be sufficient for most cases though as compiling parsers as modules isn't a common choice.

Signed-off-by Brian Norris <[email protected]>

Signed-off-by Rafał Miłecki <[email protected]>

Tested-by Peter Rosin <[email protected]>

---
This is based on Brian's patches:
[RFC PATCH 4/7] mtd: add of_match_mtd_parser() and of_mtd_match_mtd_parser() helpers [RFC PATCH 6/7] RFC: mtd: partitions: enable of_match_table matching

---
 drivers/mtd/mtdpart.c          | 116 +++++++++++++++++++++++++++++++++++++----
 include/linux/mtd/partitions.h |   1 +
 2 files changed, 108 insertions(+), 9 deletions(-)

drivers/mtd/mtdpart.c +107 -9

index 76cd21d1171b..105fe2c7729c 100644
@@ -30,6 +30,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/err.h>
+#include <linux/of.h>
 
 #include "mtdcore.h"

@@ -861,6 +862,92 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser,
 }
 
 /**
+ * mtd_part_get_compatible_parser - find MTD parser by a compatible string
+ *
+ * @compat: compatible string describing partitions in a device tree
+ *
+ * MTD parsers can specify supported partitions by providing a table of
+ * compatibility strings. This function finds a parser that advertises support
+ * for a passed value of "compatible".
+ */
+static struct mtd_part_parser *mtd_part_get_compatible_parser(const char *compat)
+{
+	struct mtd_part_parser *p, *ret = NULL;
+
+	spin_lock(&part_parser_lock);
+
+	list_for_each_entry(p, &part_parsers, list) {
+		const struct of_device_id *matches;
+
+		matches = p->of_match_table;
+		if (!matches)
+			continue;
+
+		for (; matches->compatible[0]; matches++) {
+			if (!strcmp(matches->compatible, compat) &&
+			    try_module_get(p->owner)) {
+				ret = p;
+				break;
+			}
+		}
+
+		if (ret)
+			break;
+	}
+
+	spin_unlock(&part_parser_lock);
+
+	return ret;
+}
+
+static int mtd_part_of_parse(struct mtd_info *master,
+			     struct mtd_partitions *pparts)
+{
+	struct mtd_part_parser *parser;
+	struct device_node *np;
+	struct property *prop;
+	const char *compat;
+	const char *fixed = "ofpart";
+	int ret, err = 0;
+
+	np = of_get_child_by_name(mtd_get_of_node(master), "partitions");
+	of_property_for_each_string(np, "compatible", prop, compat) {
+		parser = mtd_part_get_compatible_parser(compat);
+		if (!parser)
+			continue;
+		ret = mtd_part_do_parse(parser, master, pparts, NULL);
+		if (ret > 0) {
+			of_node_put(np);
+			return ret;
+		}
+		mtd_part_parser_put(parser);
+		if (ret < 0 && !err)
+			err = ret;
+	}
+	of_node_put(np);
+
+	/*
+	 * For backward compatibility we have to try the "ofpart"
+	 * parser. It supports old DT format with partitions specified as a
+	 * direct subnodes of a flash device DT node without any compatibility
+	 * specified we could match.
+	 */
+	parser = mtd_part_parser_get(fixed);
+	if (!parser && !request_module("%s", fixed))
+		parser = mtd_part_parser_get(fixed);
+	if (parser) {
+		ret = mtd_part_do_parse(parser, master, pparts, NULL);
+		if (ret > 0)
+			return ret;
+		mtd_part_parser_put(parser);
+		if (ret < 0 && !err)
+			err = ret;
+	}
+
+	return err;
+}
+
+/**
  * parse_mtd_partitions - parse MTD partitions
  * @master: the master partition (describes whole MTD device)
  * @types: names of partition parsers to try or %NULL

@@ -892,19 +979,30 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		types = default_mtd_part_types;
 
 	for ( ; *types; types++) {
-		pr_debug("%s: parsing partitions %s\n", master->name, *types);
-		parser = mtd_part_parser_get(*types);
-		if (!parser && !request_module("%s", *types))
+		/*
+		 * ofpart is a special type that means OF partitioning info
+		 * should be used. It requires a bit different logic so it is
+		 * handled in a separated function.
+		 */
+		if (!strcmp(*types, "ofpart")) {
+			ret = mtd_part_of_parse(master, pparts);
+		} else {
+			pr_debug("%s: parsing partitions %s\n", master->name,
+				 *types);
 			parser = mtd_part_parser_get(*types);
-		pr_debug("%s: got parser %s\n", master->name,
-			 parser ? parser->name : NULL);
-		if (!parser)
-			continue;
-		ret = mtd_part_do_parse(parser, master, pparts, data);
+			if (!parser && !request_module("%s", *types))
+				parser = mtd_part_parser_get(*types);
+			pr_debug("%s: got parser %s\n", master->name,
+				parser ? parser->name : NULL);
+			if (!parser)
+				continue;
+			ret = mtd_part_do_parse(parser, master, pparts, data);
+			if (ret <= 0)
+				mtd_part_parser_put(parser);
+		}
 		/* Found partitions! */
 		if (ret > 0)
 			return 0;
-		mtd_part_parser_put(parser);
 		/*
 		 * Stash the first error we see; only report it if no parser
 		 * succeeds

include/linux/mtd/partitions.h +1 -0

index c4beb70dacbd..11cb0c50cd84 100644
@@ -77,6 +77,7 @@ struct mtd_part_parser {
 	struct list_head list;
 	struct module *owner;
 	const char *name;
+	const struct of_device_id *of_match_table;
 	int (*parse_fn)(struct mtd_info *, const struct mtd_partition **,
 			struct mtd_part_parser_data *);
 	void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);

Signature · 6 lines

2.11.0


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

Rafał Miłecki

2018-03-14 12:10 UTC #10 lore raw

PATCH v10 2/3 mtd: rename "ofpart" parser to "fixed-partitions" as it fits it better

In reply to Rafał Miłecki

From: Rafał Miłecki <[email protected]>

Signed-off-by Rafał Miłecki <[email protected]>

---
 drivers/mtd/mtdpart.c |  4 ++--
 drivers/mtd/ofpart.c  | 11 ++++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

drivers/mtd/mtdpart.c +2 -2

index 105fe2c7729c..cdb1f496ab95 100644
@@ -907,7 +907,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
 	struct device_node *np;
 	struct property *prop;
 	const char *compat;
-	const char *fixed = "ofpart";
+	const char *fixed = "fixed-partitions";
 	int ret, err = 0;
 
 	np = of_get_child_by_name(mtd_get_of_node(master), "partitions");

@@ -927,7 +927,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
 	of_node_put(np);
 
 	/*
-	 * For backward compatibility we have to try the "ofpart"
+	 * For backward compatibility we have to try the "fixed-partitions"
 	 * parser. It supports old DT format with partitions specified as a
 	 * direct subnodes of a flash device DT node without any compatibility
 	 * specified we could match.

drivers/mtd/ofpart.c +6 -5

index 6bdf4e525677..9f497315e65d 100644
@@ -25,9 +25,9 @@ static bool node_has_compatible(struct device_node *pp)
 	return of_get_property(pp, "compatible", NULL);
 }
 
-static int parse_ofpart_partitions(struct mtd_info *master,
-				   const struct mtd_partition **pparts,
-				   struct mtd_part_parser_data *data)
+static int parse_fixed_partitions(struct mtd_info *master,
+				  const struct mtd_partition **pparts,
+				  struct mtd_part_parser_data *data)
 {
 	struct mtd_partition *parts;
 	struct device_node *mtd_node;

@@ -141,8 +141,8 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 }
 
 static struct mtd_part_parser ofpart_parser = {
-	.parse_fn = parse_ofpart_partitions,
-	.name = "ofpart",
+	.parse_fn = parse_fixed_partitions,
+	.name = "fixed-partitions",
 };
 
 static int parse_ofoldpart_partitions(struct mtd_info *master,

@@ -229,4 +229,5 @@ MODULE_AUTHOR("Vitaly Wool, David Gibson");
  * with the same name. Since we provide the ofoldpart parser, we should have
  * the corresponding alias.
  */
+MODULE_ALIAS("fixed-partitions");
 MODULE_ALIAS("ofoldpart");

Signature · 6 lines

2.11.0


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

Rafał Miłecki

2018-03-14 12:10 UTC #11 lore raw

PATCH v10 3/3 mtd: ofpart: add of_match_table with "fixed-partitions"

In reply to Rafał Miłecki

From: Rafał Miłecki <[email protected]>

This matches existing bindings documentation.

Signed-off-by Rafał Miłecki <[email protected]>

Reviewed-by Brian Norris <[email protected]>

Tested-by Brian Norris <[email protected]>

---
 drivers/mtd/ofpart.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/mtd/ofpart.c +7 -0

index 9f497315e65d..615f8c173162 100644
@@ -140,9 +140,16 @@ static int parse_fixed_partitions(struct mtd_info *master,
 	return ret;
 }
 
+static const struct of_device_id parse_ofpart_match_table[] = {
+	{ .compatible = "fixed-partitions" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, parse_ofpart_match_table);
+
 static struct mtd_part_parser ofpart_parser = {
 	.parse_fn = parse_fixed_partitions,
 	.name = "fixed-partitions",
+	.of_match_table = parse_ofpart_match_table,
 };
 
 static int parse_ofoldpart_partitions(struct mtd_info *master,

Signature · 6 lines

2.11.0


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

Richard Weinberger

maintainer Reviewed-by on the v10 series

2018-03-26 11:28 UTC #12 lore raw

In reply to Rafał Miłecki

Am Mittwoch, 14. März 2018, 13:10:41 CEST schrieb Rafał Miłecki:

Quoted 34 lines

From: Rafał Miłecki <[email protected]>
This patchset provides a proper support for flash device DT node with "partitions" subnode using "compatible" property. It's already documented in the: Documentation/devicetree/bindings/mtd/partition.txt
We believed that version 7 was ready to go, but soon after landing in the linux-next we got a regression report from Peter. Later versions takes a safe path by:
1) Respecting parsers order as specified in the default/driver-provided list.
2) Looking at "compatible" property only when "ofpart" type gets speciied.
Version 9 was successfully tested by Peter and version 10 just squashes two commits into a one.
I double-checked the code and cannot think of any regression this could cause. I also hope this design (roughly discussed with Boris) can be acceptable for the mtd subsystem.
The most important patch from this patchset (1/3) was reviewed by Rob in the
[PATCH V6 1/2] mtd: partitions: add of_match_table parser matching
https://patchwork.ozlabs.org/comment/1831551/
If possible this is a 4.17 material.
Rafał Miłecki (3):
mtd: partitions: add of_match_table parser matching for the "ofpart"
type
mtd: rename "ofpart" parser to "fixed-partitions" as it fits it better
mtd: ofpart: add of_match_table with "fixed-partitions"

Quoted 5 lines

 drivers/mtd/mtdpart.c          | 116
+++++++++++++++++++++++++++++++++++++---- drivers/mtd/ofpart.c           | 
18 +++++--
 include/linux/mtd/partitions.h |   1 +
 3 files changed, 121 insertions(+), 14 deletions(-)

Series looks good to me.

Reviewed-by Richard Weinberger <[email protected]>

Thanks,
//richard

Boris Brezillon

Applied

2018-03-27 09:41 UTC #13 lore raw

In reply to Rafał Miłecki

Quoted 21 lines · On Wed, 14 Mar 2018 13:10:41 +0100 Rafał Miłecki <[email protected]> wrote:

From: Rafał Miłecki <[email protected]>
This patchset provides a proper support for flash device DT node with "partitions" subnode using "compatible" property. It's already documented in the: Documentation/devicetree/bindings/mtd/partition.txt
We believed that version 7 was ready to go, but soon after landing in the linux-next we got a regression report from Peter. Later versions takes a safe path by:
1) Respecting parsers order as specified in the default/driver-provided list.
2) Looking at "compatible" property only when "ofpart" type gets speciied.
Version 9 was successfully tested by Peter and version 10 just squashes two commits into a one.
I double-checked the code and cannot think of any regression this could cause. I also hope this design (roughly discussed with Boris) can be acceptable for the mtd subsystem.

Quoted 6 lines

The most important patch from this patchset (1/3) was reviewed by Rob in the
[PATCH V6 1/2] mtd: partitions: add of_match_table parser matching
https://patchwork.ozlabs.org/comment/1831551/
If possible this is a 4.17 material.

Applied, finally. I'm still convinced putting this information in the "partitions" node compatible is a bad idea because it adds extra complexity to something that was already quite complex, but you won!

Note that Rob had comments on [1] that you never answered, so I'm not even sure when we'll start seeing boards using this infrastructure.

[1]http://patchwork.ozlabs.org/patch/859690/

Signature · 7 lines

Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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