[RESEND,1/4] dt-bindings: mtd: make partitions doc a bit more generic

From: Brian Norris <computersforpeace@gmail.com>

Currently the only documented partitioning is "fixed-partitions" but
there are more methods in use that we may want to support in the future.
Mention them and make it clear Fixed Partitions are just a single case.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/mtd/partition.txt          | 28 +++++++++++++++++-----
 1 file changed, 22 insertions(+), 6 deletions(-)

+ others

Hi Rafał,

Thanks for drudging up my old work. This is something that's been stuck
too far down in my stack of TODOs that it essentially timed out...

On Thu, Apr 20, 2017 at 03:57:28PM +0200, Rafał Miłecki wrote:
> From: Brian Norris <computersforpeace@gmail.com>
> 
> Currently the only documented partitioning is "fixed-partitions" but
> there are more methods in use that we may want to support in the future.
> Mention them and make it clear Fixed Partitions are just a single case.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---

I mentioned the missing changelog on IRC, but to fill in the gaps:
essentially you've dropped the partition parser and associated bindings
I had in my series (for the 'Google FMAP' format). That's fine, but I
just wanted to note it.

And with that, I think you've not quite nailed the purpose of my
original patch. This now seems to suggest there are other potential
bindings here, but then you leave the reader hanging. One note to that
effect below. If that's the only objection, then I can make the
additions myself when applying.

>  .../devicetree/bindings/mtd/partition.txt          | 28 +++++++++++++++++-----
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
> index 81a224da63be..27593adc45c2 100644
> --- a/Documentation/devicetree/bindings/mtd/partition.txt
> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
> @@ -1,29 +1,45 @@
> -Representing flash partitions in devicetree
> +Flash partitions in device tree
> +===============================
>  
> -Partitions can be represented by sub-nodes of an mtd device. This can be used
> +Flash devices can be partitioned into one or more functional ranges (e.g. "boot
> +code", "nvram", "kernel").
> +
> +Different devices may be partitioned in a different ways. Some may use a fixed
> +flash layout set at production time. Some may use on-flash table that describes
> +the geometry and naming/purpose of each functional region. It is also possible
> +to see these methods mixed.
> +
> +To assist system software in locating partitions, we provide a binding to
> +describe which method is used for a given flash.


We've suggested above that there may be "different ways" (fixed vs.
on-flash tables) to partition, but then we still only describe one way.
Maybe we can just paste something like this as a caveat before moving
on?

"We currently only document a binding for fixed layouts."

We can delete that in the presumed follow-up that proposes the
"on-flash" parser bindings. Sound OK?

Brian

> +
> +
> +Fixed Partitions
> +================
> +
> +Partitions can be represented by sub-nodes of a flash device. This can be used
>  on platforms which have strong conventions about which portions of a flash are
>  used for what purposes, but which don't use an on-flash partition table such
>  as RedBoot.
>  
> -The partition table should be a subnode of the mtd node and should be named
> +The partition table should be a subnode of the flash node and should be named
>  'partitions'. This node should have the following property:
>  - compatible : (required) must be "fixed-partitions"
>  Partitions are then defined in subnodes of the partitions node.
>  
> -For backwards compatibility partitions as direct subnodes of the mtd device are
> +For backwards compatibility partitions as direct subnodes of the flash device are
>  supported. This use is discouraged.
>  NOTE: also for backwards compatibility, direct subnodes that have a compatible
>  string are not considered partitions, as they may be used for other bindings.
>  
>  #address-cells & #size-cells must both be present in the partitions subnode of the
> -mtd device. There are two valid values for both:
> +flash device. There are two valid values for both:
>  <1>: for partitions that require a single 32-bit cell to represent their
>       size/address (aka the value is below 4 GiB)
>  <2>: for partitions that require two 32-bit cells to represent their
>       size/address (aka the value is 4 GiB or greater).
>  
>  Required properties:
> -- reg : The partition's offset and size within the mtd bank.
> +- reg : The partition's offset and size within the flash
>  
>  Optional properties:
>  - label : The label / name for this partition.  If omitted, the label is taken
> -- 
> 2.11.0
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Rafał Miłecki <rafal@milecki.pl>

My recent work on adding wide support for linux,part-probe was reviewed and
kind of Nack-ed, but fortunately I was pointed to the old (!) patchset from
Brian doing similar thing in a cleaner way.

This patchset picks the important changes from Brian, cleans them and rebases.

Original patches:
(not picked) [RFC PATCH 1/7] mtd: move partition parsers to drivers/mtd/partitions/
(not picked) [RFC PATCH 2/7] mtd: move partition parsers' Kconfig under a sub-menu
(partially)  [RFC PATCH 3/7] doc: dt: mtd: partition: add on-flash format binding
(picked)     [RFC PATCH 4/7] mtd: add of_match_mtd_parser() and of_mtd_match_mtd_parser() helpers
(partially)  [RFC PATCH 5/7] mtd: partitions: factor out "match by name" handling
(picked)     [RFC PATCH 6/7] RFC: mtd: partitions: enable of_match_table matching
(not picked) [RFC PATCH 7/7] mtd: partitions: add Google's FMAP partition parser

At this point this simply adds a full support for "fixed-partitions" binding.
It should also make adding new bindings (like Google's FMAP) easier in the
future.

I've successfully tested this with bcm47xxsflash driver on Tenda AC9 device. I
used following DT node to get "ofpart" driver parse & register my partitions.

&bcma-sflash {
	partitions {
		compatible = "fixed-partitions";
		#address-cells = <1>;
		#size-cells = <1>;

		partition@0 {
			label = "cfe";
			reg = <0x0000000 0x40000>;
			read-only;
		};

		firmware@40000 {
			label = "firmware";
			reg = <0x40000 0x7f0000>;
		};
	};
};

V2: Modify patch 1/4
    Include list of original patches in 0/4
    Include changelog in every patch
    Add Brian's tags (Acked/Reviewed/Tested)

Brian Norris (3):
  dt-bindings: mtd: make partitions doc a bit more generic
  mtd: partitions: factor out code calling parser
  mtd: partitions: add of_match_table parser matching

Rafał Miłecki (1):
  mtd: ofpart: add of_match_table with "fixed-partitions"

 .../devicetree/bindings/mtd/partition.txt          | 30 ++++++--
 drivers/mtd/mtdpart.c                              | 80 +++++++++++++++++++---
 drivers/mtd/ofpart.c                               |  7 ++
 include/linux/mtd/partitions.h                     |  1 +
 4 files changed, 103 insertions(+), 15 deletions(-)

Hi,

On 24 April 2017 at 14:41, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Brian Norris <computersforpeace@gmail.com>
>
> 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@gmail.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Acked-by: Brian Norris <computersforpeac@gmail.com>
> ---
> 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
> ---
>  drivers/mtd/mtdpart.c          | 47 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/partitions.h |  1 +
>  2 files changed, 48 insertions(+)
>
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 73c52f1a2e4c..d0cb1a892ed2 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -861,6 +861,41 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser,
>         return ret;
>  }
>
> +static bool of_mtd_match_mtd_parser(struct mtd_info *mtd,
> +                                   struct mtd_part_parser *parser)
> +{
> +       struct device_node *np;
> +       bool ret;
> +
> +       np = mtd_get_of_node(mtd);
> +       np = of_get_child_by_name(np, "partitions");
> +
> +       ret = !!of_match_node(parser->of_match_table, np);
> +
> +       of_node_put(np);
> +
> +       return ret;
> +}
> +
> +static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd)
> +{
> +       struct mtd_part_parser *p, *ret = NULL;
> +
> +       spin_lock(&part_parser_lock);
> +
> +       list_for_each_entry(p, &part_parsers, list) {
> +               if (of_mtd_match_mtd_parser(mtd, p) &&
> +                               try_module_get(p->owner)) {
> +                       ret = p;
> +                       break;
> +               }
> +       }


Hm, maybe iterate over the compatibles, so parsers matching the most
specific compatible get precedence in case there is more than one
compatible? Currently it will match the first one that matches any
compatible, and registration order of parsers can change that. I'm
thinking of parsers that partially rely on fixed, unprobable layouts,
so can use "fixed-partitions" as a fallback compatible.

E.g. having something like this

partitions {
        compatible = "sample,custom-layout", "fixed-partitions";

        bootloader@0 { ...  };

        firmware@10000 { .... }; /* will be split by the parser */

        extra@780000 { .... }; /* partition the on-flash format can't specify */
};

Where you will still be able to write an image raw to the image
partition even if the "custom-layout"-parser isn't present/enabled,
but if it is present, it should always be used.


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

On 04/24/2017 05:31 PM, Jonas Gorski wrote:
> On 24 April 2017 at 14:41, Rafał Miłecki <zajec5@gmail.com> wrote:
>> From: Brian Norris <computersforpeace@gmail.com>
>>
>> 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@gmail.com>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> Acked-by: Brian Norris <computersforpeac@gmail.com>
>> ---
>> 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
>> ---
>>  drivers/mtd/mtdpart.c          | 47 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/partitions.h |  1 +
>>  2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
>> index 73c52f1a2e4c..d0cb1a892ed2 100644
>> --- a/drivers/mtd/mtdpart.c
>> +++ b/drivers/mtd/mtdpart.c
>> @@ -861,6 +861,41 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser,
>>         return ret;
>>  }
>>
>> +static bool of_mtd_match_mtd_parser(struct mtd_info *mtd,
>> +                                   struct mtd_part_parser *parser)
>> +{
>> +       struct device_node *np;
>> +       bool ret;
>> +
>> +       np = mtd_get_of_node(mtd);
>> +       np = of_get_child_by_name(np, "partitions");
>> +
>> +       ret = !!of_match_node(parser->of_match_table, np);
>> +
>> +       of_node_put(np);
>> +
>> +       return ret;
>> +}
>> +
>> +static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd)
>> +{
>> +       struct mtd_part_parser *p, *ret = NULL;
>> +
>> +       spin_lock(&part_parser_lock);
>> +
>> +       list_for_each_entry(p, &part_parsers, list) {
>> +               if (of_mtd_match_mtd_parser(mtd, p) &&
>> +                               try_module_get(p->owner)) {
>> +                       ret = p;
>> +                       break;
>> +               }
>> +       }
>
>
> Hm, maybe iterate over the compatibles, so parsers matching the most
> specific compatible get precedence in case there is more than one
> compatible? Currently it will match the first one that matches any
> compatible, and registration order of parsers can change that. I'm
> thinking of parsers that partially rely on fixed, unprobable layouts,
> so can use "fixed-partitions" as a fallback compatible.
>
> E.g. having something like this
>
> partitions {
>         compatible = "sample,custom-layout", "fixed-partitions";
>
>         bootloader@0 { ...  };
>
>         firmware@10000 { .... }; /* will be split by the parser */
>
>         extra@780000 { .... }; /* partition the on-flash format can't specify */
> };
>
> Where you will still be able to write an image raw to the image
> partition even if the "custom-layout"-parser isn't present/enabled,
> but if it is present, it should always be used.

I see the point, but I'm afraid we're lacking some DT helper for this. See
below for the function I wrote (and I'm not proud of) - compile tested only.

I think we would need a new helper similar to the of_match_node:
1) Taking const struct of_device_id *matches
2) Taking const struct device_node *node
but returning a score of the best match.

DT guys: any comment on this? Rob?

Would this be acceptable to:
1) Take this patch as is as Linux current doesn't support other bindings
2) Work on DT helper + mtd modification in a separated patchset?

static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd)
{
	struct mtd_part_parser *p, *ret = NULL;
	struct device_node *np;
	struct property *prop;
	const char *cp;

	np = mtd_get_of_node(mtd);
	np = of_get_child_by_name(np, "partitions");
	if (!np)
		return NULL;

	spin_lock(&part_parser_lock);

	of_property_for_each_string(np, "compatible", prop, cp) {
		list_for_each_entry(p, &part_parsers, list) {
			const struct of_device_id *matches;

			for (matches = p->of_match_table;
			     matches->name[0] || matches->type[0] || matches->compatible[0];
			     matches++) {
				if (!of_compat_cmp(cp, matches->compatible, strlen(matches->compatible)) &&
				    try_module_get(p->owner)) {
					ret = p;
					break;
				}
			}

			if (ret)
				break;
		}

		if (ret)
			break;
	}

	spin_unlock(&part_parser_lock);

	of_node_put(np);

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