[PATCH V4 1/2] mtd: partitions: add of_match_table parser matching

[PATCH V4 1/2] mtd: partitions: add of_match_table parser matching

[Rafał Miłecki]

From: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Partition parsers can now provide an of_match_table to enable
flash<-->parser matching via device tree.

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 <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
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
---
 drivers/mtd/mtdpart.c          | 65 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/partitions.h |  1 +
 2 files changed, 66 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 2ad9493703f9..239d5e6e62ed 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -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"
 
@@ -885,6 +886,60 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser,
 	return ret;
 }
 
+static int of_mtd_match_mtd_parser(struct mtd_info *mtd,
+				   struct mtd_part_parser *parser)
+{
+	const struct of_device_id *matches;
+	struct device_node *np;
+	int score = 0;
+
+	matches = parser->of_match_table;
+	if (!matches)
+		return false;
+
+	np = mtd_get_of_node(mtd);
+	np = of_get_child_by_name(np, "partitions");
+	if (!np)
+		return false;
+
+	for (; matches->name[0] || matches->type[0] || matches->compatible[0];
+	     matches++) {
+		if (!matches->compatible[0])
+			continue;
+		score = max(score,
+			    of_device_is_compatible(np, matches->compatible));
+	}
+
+	of_node_put(np);
+
+	return score;
+}
+
+static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd)
+{
+	struct mtd_part_parser *p, *ret = NULL;
+	struct mtd_part_parser *best_parser = NULL;
+	int best_score = 0;
+
+	spin_lock(&part_parser_lock);
+
+	list_for_each_entry(p, &part_parsers, list) {
+		int score = of_mtd_match_mtd_parser(mtd, p);
+
+		if (score > best_score) {
+			best_score = score;
+			best_parser = p;
+		}
+	}
+
+	if (best_parser && try_module_get(best_parser->owner))
+		ret = best_parser;
+
+	spin_unlock(&part_parser_lock);
+
+	return ret;
+}
+
 /**
  * parse_mtd_partitions - parse MTD partitions
  * @master: the master partition (describes whole MTD device)
@@ -913,6 +968,16 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 	struct mtd_part_parser *parser;
 	int ret, err = 0;
 
+	parser = mtd_part_get_parser_by_of(master);
+	if (parser) {
+		ret = mtd_part_do_parse(parser, master, pparts, data);
+		if (ret > 0)
+			return 0;
+		mtd_part_parser_put(parser);
+		if (ret < 0)
+			err = ret;
+	}
+
 	if (!types)
 		types = default_mtd_part_types;
 
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index c4beb70dacbd..11cb0c50cd84 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -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);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 2/2] mtd: ofpart: add of_match_table with "fixed-partitions"

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

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 <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
Reviewed-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/ofpart.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index 2861c7079d7b..fb6f3df40e94 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -140,9 +140,16 @@ static int parse_ofpart_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_ofpart_partitions,
 	.name = "ofpart",
+	.of_match_table = parse_ofpart_match_table,
 };
 
 static int parse_ofoldpart_partitions(struct mtd_info *master,
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 1/2] mtd: partitions: add of_match_table parser matching

[Jonas Gorski]

Hi,

On 25 June 2017 at 01:10, Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Partition parsers can now provide an of_match_table to enable
> flash<-->parser matching via device tree.
>
> 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 <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

I gave this and Rafał's recent patches a spin and split out
bcm63xxpart's imagetag parsing into its own parser as a partition
parser assigned to the imagetag partition, then added an
of_match_table to bcm63xxpart, and it worked beautifully:

[    1.111302] m25p80 spi1.0: s25fl129p1 (16384 Kbytes)
[    1.117222] bcm63xxpart: Partition 0 is CFE offset 0 and length 10000
[    1.127313] bcm63xxpart: Partition 1 is nvram offset ff0000 and length 10000
[    1.134604] bcm63xxpart: Partition 2 is linux offset 10000 and length fe0000
[    1.141927] 3 bcm63xxpart partitions found on MTD device spi1.0
[    1.148032] Creating 3 MTD partitions on "spi1.0":
[    1.152945] 0x000000000000-0x000000010000 : "CFE"
[    1.161004] 0x000000ff0000-0x000001000000 : "nvram"
[    1.169777] 0x000000010000-0x000000ff0000 : "linux"
[    1.179489] parser_bcm63xx_imagetag: rootfs: CFE image tag found at
0x0 with version 6, board type 96328avng
[    1.189671] parser_bcm63xx_imagetag: Partition 0 is kernel offset
100 and length 151f52
[    1.197902] parser_bcm63xx_imagetag: Partition 1 is rootfs offset
152052 and length e8dfae
[    1.206405] parser_bcm63xx_imagetag: Spare partition is offset
350004 and length c8fffc
[    1.214738] 2 bcm63xx-imagetag partitions found on MTD device linux
[    1.221201] Creating 2 MTD partitions on "linux":
[    1.226032] 0x000000000100-0x000000152052 : "kernel"
[    1.234445] 0x000000152052-0x000000fe0000 : "rootfs"

Things I did:
1) having just the compatible for bcm63xxpart: bcm63xxpart was used.
2) bcm63xxpart, then fixed-partitions as two compatible strings:
bcm63xxpart was used.
3) fixed-partitons, then bcm63xxpart: ofpart was used.
4) non-existent compatible, then fixed-partitons: ofpart was used.

then I ran out of ideas to test.

I guess this might be enough for a

Tested-by: Jonas Gorski <jonas.gorski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


Regards
Jonas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 1/2] mtd: partitions: add of_match_table parser matching

[Brian Norris]

Hi,

On Sun, Jun 25, 2017 at 01:10:24AM +0200, Rafał Miłecki wrote:
> From: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Partition parsers can now provide an of_match_table to enable
> flash<-->parser matching via device tree.
> 
> 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 <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> ---
> 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
> ---
>  drivers/mtd/mtdpart.c          | 65 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/partitions.h |  1 +
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 2ad9493703f9..239d5e6e62ed 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -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"
>  
> @@ -885,6 +886,60 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser,
>  	return ret;
>  }
>  
> +static int of_mtd_match_mtd_parser(struct mtd_info *mtd,
> +				   struct mtd_part_parser *parser)
> +{
> +	const struct of_device_id *matches;
> +	struct device_node *np;
> +	int score = 0;
> +
> +	matches = parser->of_match_table;
> +	if (!matches)
> +		return false;
> +
> +	np = mtd_get_of_node(mtd);
> +	np = of_get_child_by_name(np, "partitions");
> +	if (!np)
> +		return false;

This function returns int, but you're returning 'false'. Should probably
be 0, or negative.

> +
> +	for (; matches->name[0] || matches->type[0] || matches->compatible[0];
> +	     matches++) {
> +		if (!matches->compatible[0])
> +			continue;
> +		score = max(score,
> +			    of_device_is_compatible(np, matches->compatible));
> +	}
> +
> +	of_node_put(np);
> +
> +	return score;
> +}
> +
> +static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd)
> +{
> +	struct mtd_part_parser *p, *ret = NULL;
> +	struct mtd_part_parser *best_parser = NULL;
> +	int best_score = 0;
> +
> +	spin_lock(&part_parser_lock);
> +
> +	list_for_each_entry(p, &part_parsers, list) {
> +		int score = of_mtd_match_mtd_parser(mtd, p);
> +
> +		if (score > best_score) {
> +			best_score = score;
> +			best_parser = p;
> +		}
> +	}
> +
> +	if (best_parser && try_module_get(best_parser->owner))
> +		ret = best_parser;

Unfortunately, this only tries a single parser (the "best" in the
compatible list). But what if that parser doesn't match? I thought the
idea was to fall back to the others. e.g., if this was like a block
device, we might have something like 'compatible = "gpt", "mbr";', and
if we don't find a GPT descriptor, we fall back to MBR.

I believe the correct algorithm for this is:

 for each c in compatible: // in order
   for each p in parsers:
     if p matches c:
       ret = do_parse(p)
       if ret > 0:
         terminate(success)

Which is essentially "try parsers that match each compatible property,
starting with the first". (You could also do this by sorting the parser
list, but that would just be extra complex.)

If I understand right, yours is like sorting the parser list and only
trying the first one (i.e., the "max score").

Brian

> +
> +	spin_unlock(&part_parser_lock);
> +
> +	return ret;
> +}
> +
>  /**
>   * parse_mtd_partitions - parse MTD partitions
>   * @master: the master partition (describes whole MTD device)
> @@ -913,6 +968,16 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  	struct mtd_part_parser *parser;
>  	int ret, err = 0;
>  
> +	parser = mtd_part_get_parser_by_of(master);
> +	if (parser) {
> +		ret = mtd_part_do_parse(parser, master, pparts, data);
> +		if (ret > 0)
> +			return 0;
> +		mtd_part_parser_put(parser);
> +		if (ret < 0)
> +			err = ret;
> +	}
> +
>  	if (!types)
>  		types = default_mtd_part_types;
>  
> diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
> index c4beb70dacbd..11cb0c50cd84 100644
> --- a/include/linux/mtd/partitions.h
> +++ b/include/linux/mtd/partitions.h
> @@ -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);
> -- 
> 2.11.0
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html