[PATCH 1/3] mtd: create a partition type device tree binding

[PATCH 1/3] mtd: create a partition type device tree binding

[Linus Walleij]

The Linux code in drivers/mtd/maps/physmap_of.c will optionally
look for the linux,part-probe binding for hints on a partition type to
look for in the MTD. It was added in commit 9d5da3a9b849
"mtd: extend physmap_of to let the device tree specify the parition probe"

This solution is too Linux-specific and depend on some
Linux kernel-internal naming conventions. We need a proper
way of describing partition types that follow the pattern set by
other device tree bindings.

Create a "partition-type" binding for this, and add "my" bindings
for "arm,arm-flash-structure" as a starter for others to follow.
A follow-on patch adds support to the Linux kernel to handle this
binding, with some infrastructure for others to use it too.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Cc: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
Reported-by: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/mtd/mtd-physmap.txt        |  2 ++
 .../devicetree/bindings/mtd/partition.txt          | 35 +++++++++++++++++++---
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
index 4a0a48bf4ecb..863560bdbb19 100644
--- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
+++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
@@ -23,6 +23,8 @@ file systems on embedded devices.
    unaligned accesses as implemented in the JFFS2 code via memcpy().
    By defining "no-unaligned-direct-access", the flash will not be
    exposed directly to the MTD users (e.g. JFFS2) any more.
+ - partition-type : a flash partition type to expect and probe for
+   on this device. See "partition.txt" for available partition types.
  - linux,mtd-name: allow to specify the mtd name for retro capability with
    physmap-flash drivers as boot loader pass the mtd partition via the old
    device name physmap-flash.
diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
index 8e5557da1955..85d45764a4b3 100644
--- a/Documentation/devicetree/bindings/mtd/partition.txt
+++ b/Documentation/devicetree/bindings/mtd/partition.txt
@@ -1,9 +1,36 @@
 Representing flash partitions in devicetree
 
-Partitions can be represented by sub-nodes of an mtd 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.
+On-device partition types:
+
+It is possible for some drivers to indicate an on-device partition type, i.e.
+partition tables, footers or other binary pattern in the flash used to
+define how the flash is partitioned. This can be done in the device tree node
+of an MTD device by specifying partition-type = "foo"; This tells the operating
+system to look for the partition type indicated.
+
+Required properties:
+- partition-type : the type of partition. Only one type can be specified.
+  Valid types are:
+  "arm,arm-flash-structure" (also called AFS)
+
+Example:
+
+flash0@40000000 {
+	/* 2 * 32MiB NOR Flash memory */
+	compatible = "arm,vexpress-flash", "cfi-flash";
+	partition-type = "arm,arm-flash-structure";
+	reg = <0x40000000 0x04000000>;
+	bank-width = <4>;
+};
+
+
+Device Tree specified partitions:
+
+If there is no specified on-device binary format, partitions can be
+represented by sub-nodes of an mtd device. This can be used on platforms which
+have strong conventions about which portions of a flash are used for what
+purposes.
+
 NOTE: if the sub-node has a compatible string, then it is not a partition.
 
 #address-cells & #size-cells must both be present in the mtd device. There are
-- 
2.4.3

--
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 1/3] mtd: create a partition type device tree binding

[Brian Norris]

+ devicetree-spec and others, since other non-driver-specific MTD
partitioning stuff has gotten directed there [1], and this binding
should probably get promoted in short order to the core mtdpart.c
partitioning

On Thu, Oct 29, 2015 at 01:52:30PM +0100, Linus Walleij wrote:
> The Linux code in drivers/mtd/maps/physmap_of.c will optionally
> look for the linux,part-probe binding for hints on a partition type to
> look for in the MTD. It was added in commit 9d5da3a9b849
> "mtd: extend physmap_of to let the device tree specify the parition probe"
> 
> This solution is too Linux-specific and depend on some
> Linux kernel-internal naming conventions. We need a proper
> way of describing partition types that follow the pattern set by
> other device tree bindings.
> 
> Create a "partition-type" binding for this, and add "my" bindings
> for "arm,arm-flash-structure" as a starter for others to follow.
> A follow-on patch adds support to the Linux kernel to handle this
> binding, with some infrastructure for others to use it too.

Overall, I like this. And thanks for expanding the explanation of DT
partitions vs. parsers. It'd be good if we make parsers like this easier
to integrate, so we see less hard-coded partitions in device tree.

> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Cc: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
> Reported-by: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/mtd-physmap.txt        |  2 ++
>  .../devicetree/bindings/mtd/partition.txt          | 35 +++++++++++++++++++---
>  2 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> index 4a0a48bf4ecb..863560bdbb19 100644
> --- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> +++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> @@ -23,6 +23,8 @@ file systems on embedded devices.
>     unaligned accesses as implemented in the JFFS2 code via memcpy().
>     By defining "no-unaligned-direct-access", the flash will not be
>     exposed directly to the MTD users (e.g. JFFS2) any more.
> + - partition-type : a flash partition type to expect and probe for
> +   on this device. See "partition.txt" for available partition types.
>   - linux,mtd-name: allow to specify the mtd name for retro capability with
>     physmap-flash drivers as boot loader pass the mtd partition via the old
>     device name physmap-flash.
> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
> index 8e5557da1955..85d45764a4b3 100644
> --- a/Documentation/devicetree/bindings/mtd/partition.txt
> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
> @@ -1,9 +1,36 @@
>  Representing flash partitions in devicetree
>  
> -Partitions can be represented by sub-nodes of an mtd 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.
> +On-device partition types:
> +
> +It is possible for some drivers to indicate an on-device partition type, i.e.
> +partition tables, footers or other binary pattern in the flash used to
> +define how the flash is partitioned. This can be done in the device tree node
> +of an MTD device by specifying partition-type = "foo"; This tells the operating
> +system to look for the partition type indicated.
> +
> +Required properties:
> +- partition-type : the type of partition. Only one type can be specified.

You're supporting this as a list property (for future expansion,
presumably), so I can only assume the "only one type" is referring to
the number of different parsers available currently, not the behavior of
the property itself? This should probably be worded differently if
that's the intention. Like, "a list of partition types ... ordered by
priority .... We currently support the following: (list only the AFS
type)" (fill in the blanks how you'd like).

Also, this patch will conflict with [1]. I'll probably take [1] soon, so
one of us will have to rebase this.

Brian

[1] http://thread.gmane.org/gmane.comp.devicetree.spec/191

> +  Valid types are:
> +  "arm,arm-flash-structure" (also called AFS)
> +
> +Example:
> +
> +flash0@40000000 {
> +	/* 2 * 32MiB NOR Flash memory */
> +	compatible = "arm,vexpress-flash", "cfi-flash";
> +	partition-type = "arm,arm-flash-structure";
> +	reg = <0x40000000 0x04000000>;
> +	bank-width = <4>;
> +};
> +
> +
> +Device Tree specified partitions:
> +
> +If there is no specified on-device binary format, partitions can be
> +represented by sub-nodes of an mtd device. This can be used on platforms which
> +have strong conventions about which portions of a flash are used for what
> +purposes.
> +
>  NOTE: if the sub-node has a compatible string, then it is not a partition.
>  
>  #address-cells & #size-cells must both be present in the mtd device. There are
> -- 
> 2.4.3
> 

Re: [PATCH 1/3] mtd: create a partition type device tree binding

[Linus Walleij]

On Thu, Oct 29, 2015 at 5:29 PM, Brian Norris
<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

>> +Required properties:
>> +- partition-type : the type of partition. Only one type can be specified.
>
> You're supporting this as a list property (for future expansion,
> presumably), so I can only assume the "only one type" is referring to
> the number of different parsers available currently, not the behavior of
> the property itself?

I was thinking that it would not be a list actually.

The reason being that if you're anyways going to the trouble of
specifying exactly what partition type is going to be used, you're
not really interested in specifying a few different ones, you know
exactly what type it is going to be.

But if you think it needs to be a list, I'll specify that, no big
deal. I'll also try to get the Linux code to handle a list then.

> Also, this patch will conflict with [1]. I'll probably take [1] soon, so
> one of us will have to rebase this.

Sure I'll rebase on whatever you say.

Yours,
Linus Walleij

Re: [PATCH 1/3] mtd: create a partition type device tree binding

[Brian Norris]

On Fri, Oct 30, 2015 at 03:00:57PM +0100, Linus Walleij wrote:
> On Thu, Oct 29, 2015 at 5:29 PM, Brian Norris
> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> >> +Required properties:
> >> +- partition-type : the type of partition. Only one type can be specified.
> >
> > You're supporting this as a list property (for future expansion,
> > presumably), so I can only assume the "only one type" is referring to
> > the number of different parsers available currently, not the behavior of
> > the property itself?
> 
> I was thinking that it would not be a list actually.
> 
> The reason being that if you're anyways going to the trouble of
> specifying exactly what partition type is going to be used, you're
> not really interested in specifying a few different ones, you know
> exactly what type it is going to be.

OK, that makes sense. I think it's still *possible* that a board might
have the option of more than one partition parser, and so they might
just include both in the DTS, but that seems unlikely and so it makes
sense not to (over)engineer for it before it's needed. Anyway, your
binding can easily be expanded in the future if needed.

> But if you think it needs to be a list, I'll specify that, no big
> deal. I'll also try to get the Linux code to handle a list then.

No, I think your proposal is OK then. It's possible there is some room
for clarification in the binding, since I was confused, but that could
mostly be attributed to my pre-existing assumptions, not a fault of the
text.

> > Also, this patch will conflict with [1]. I'll probably take [1] soon, so
> > one of us will have to rebase this.
> 
> Sure I'll rebase on whatever you say.

OK, I'll let you know.

Brian

Re: [PATCH 1/3] mtd: create a partition type device tree binding

[Rob Herring]

On Fri, Oct 30, 2015 at 12:51 PM, Brian Norris
<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Oct 30, 2015 at 03:00:57PM +0100, Linus Walleij wrote:
>> On Thu, Oct 29, 2015 at 5:29 PM, Brian Norris
>> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> >> +Required properties:
>> >> +- partition-type : the type of partition. Only one type can be specified.
>> >
>> > You're supporting this as a list property (for future expansion,
>> > presumably), so I can only assume the "only one type" is referring to
>> > the number of different parsers available currently, not the behavior of
>> > the property itself?
>>
>> I was thinking that it would not be a list actually.
>>
>> The reason being that if you're anyways going to the trouble of
>> specifying exactly what partition type is going to be used, you're
>> not really interested in specifying a few different ones, you know
>> exactly what type it is going to be.
>
> OK, that makes sense. I think it's still *possible* that a board might
> have the option of more than one partition parser, and so they might
> just include both in the DTS, but that seems unlikely and so it makes
> sense not to (over)engineer for it before it's needed. Anyway, your
> binding can easily be expanded in the future if needed.

Since we now have partitions contained in a sub node, how about using
compatible for that sub node instead.

Rob

Re: [PATCH 1/3] mtd: create a partition type device tree binding

[Brian Norris]

On Fri, Nov 06, 2015 at 08:13:13AM -0600, Rob Herring wrote:
> Since we now have partitions contained in a sub node, how about using
> compatible for that sub node instead.

That seems like a pretty good idea to me.

Brian
--
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 1/3] mtd: create a partition type device tree binding

[Linus Walleij]

On Tue, Nov 10, 2015 at 4:26 AM, Brian Norris
<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Nov 06, 2015 at 08:13:13AM -0600, Rob Herring wrote:
>> Since we now have partitions contained in a sub node, how about using
>> compatible for that sub node instead.
>
> That seems like a pretty good idea to me.

Hm! That's a clever idea. I'll take a spin on that.

Yours,
Linus Walleij

Re: [PATCH 1/3] mtd: create a partition type device tree binding

[Brian Norris]

On Fri, Nov 06, 2015 at 08:13:13AM -0600, Rob Herring wrote:
> Since we now have partitions contained in a sub node, how about using
> compatible for that sub node instead.

I see that Linus and I spoke up in agreement on this one.

I took a little look at adding of_match_table support to the core MTD
partitioning code (not sure if that's duplicating anything Linus was
attempting on his own?), and I'm observing that there's potential for
conflict with the new binding [1]. If we're going to start overloading
the 'partitions' node to support other partitioning types via
'compatible' property, then we either need to:

 (1) go back to specifying that full ofpart specifications must have
 *no* compatible property or

 (2) we should define a comptible property for the hard-coded
 partitioning case (e.g., compatible = "partitions")

IOW, I could imagine a new partition parser that needs a DT like this:

	flash@xxx {
		compatible = "vendor,flash-type";
		...
		partitions {
			compatible = "some-new-partition-parser";
			...
			subnode {
				// "some-new-partition-parser" might
				// need to put something here
			};
		};
	};

But currently, the binding would say that 'subnode' must be a partition,
even if it's really something else auxiliary to
"some-new-partition-parser" [2].

If we went with option (1), then we'd just have ofpart.c see that
'partitions' has a compatible property and bail out. That seems kinda
hacky.

If we went with option (2), then ofpart.c could just check only for
'compatible = "partitions"' (or similar), and if not found bail out.

I think option (2) makes more sense. But it would require an update to
the binding and code for 4.4, since [1] was only introduced during this
release cycle.

Brian

[1] fe2585e9c29a ("doc: dt: mtd: support partitions in a special 'partitions' subnode")

[2] Possibilities: something relevant to partition splitting. See some
previous work, which I haven't gotten around to fully addressing yet,
but can hopefully be rolled into this work:

http://patchwork.ozlabs.org/patch/476373/
http://patchwork.ozlabs.org/patch/473364/
http://patchwork.ozlabs.org/patch/475988/

Re: [PATCH 1/3] mtd: create a partition type device tree binding

[Linus Walleij]

On Fri, Nov 13, 2015 at 11:00 PM, Brian Norris
<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

(...)
>  (2) we should define a comptible property for the hard-coded
>  partitioning case (e.g., compatible = "partitions")
(...)
> If we went with option (2), then ofpart.c could just check only for
> 'compatible = "partitions"' (or similar), and if not found bail out.

So this "hard-coded partitioning case" the case is where all partitions
are defined in the device tree as described in
Documentation/devicetree/bindings/mtd/partition.txt ?

Or is it a way to indicate that this array
static const char * const part_probe_types_def[] = {
        "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
in physmap_of.c should be used?

Sorry if I don't fully follow, I'm a bit stupid when it comes to the MTD
helicopter  view of the situation.... :(

> I think option (2) makes more sense. But it would require an update to
> the binding and code for 4.4, since [1] was only introduced during this
> release cycle.

Does that mean all old device trees that specify no compatible
string all of a sudden stop working? We can't break the DT ABI, so I
guess not.

A bit confused here, I can't really see what I should do with the patch...

Yours,
Linus Walleij

Re: [PATCH 1/3] mtd: create a partition type device tree binding

[Brian Norris]

On Sat, Nov 14, 2015 at 11:46:59AM +0100, Linus Walleij wrote:
> On Fri, Nov 13, 2015 at 11:00 PM, Brian Norris
> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> (...)
> >  (2) we should define a comptible property for the hard-coded
> >  partitioning case (e.g., compatible = "partitions")
> (...)
> > If we went with option (2), then ofpart.c could just check only for
> > 'compatible = "partitions"' (or similar), and if not found bail out.
> 
> So this "hard-coded partitioning case" the case is where all partitions
> are defined in the device tree as described in
> Documentation/devicetree/bindings/mtd/partition.txt ?

Right.

> Or is it a way to indicate that this array
> static const char * const part_probe_types_def[] = {
>         "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
> in physmap_of.c should be used?

No. At this point, I would consider that to be a legacy method. We still
have to support this for many cases (including the non-DT case; but
hopefully we can do better than that soon), and that would be an option.

> Sorry if I don't fully follow, I'm a bit stupid when it comes to the MTD
> helicopter  view of the situation.... :(

Yeah...sorry if I wasn't too clear. And I definitely don't blame you for
not understanding the mess that MTD often is :(

> > I think option (2) makes more sense. But it would require an update to
> > the binding and code for 4.4, since [1] was only introduced during this
> > release cycle.
> 
> Does that mean all old device trees that specify no compatible
> string all of a sudden stop working? We can't break the DT ABI, so I
> guess not.

No, that's not what I was intending. The binding before commmit
fe2585e9c29a ("doc: dt: mtd: support partitions in a special
'partitions' subnode") should still stay working as-is. That is, we
don't mess with the way things worked for anything that doesn't have a
'partitions' subnode. But now that we have a 'partitions' subnode (in
4.4-rc1), I'm just suggesting that we enforce this always have a
compatible property, so we can be more clear on the difference between:

	partitions {
		// do I have a
		// compatible = "partitons";
		// here?

		partition@0 {
			label = "foo-partition";
			reg = <0 0x100000>;
		};
	};

and

	partitions {
		compatible = "arm,arm-flash-structure";

		subnode {
			// what if we need something here eventually?
		};
	};

This would require some modifications to partitions.txt and to
drivers/mtd/ofpart.c.

> A bit confused here, I can't really see what I should do with the patch...

Hopefully that cleared up a bit? The code changes for my suggestion
would just be something like this, I think. (Not tested in any way.)

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index 669c3452f278..6811bc5440a4 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -50,6 +50,10 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 			master->name, mtd_node->full_name);
 		ofpart_node = mtd_node;
 		dedicated = false;
+	} else {
+		/* The "partitions" subnode may belong to some other parser */
+		if (!of_device_is_compatible(ofpart_node, "partitions"))
+			return 0;
 	}
 
 	/* First count the subnodes */

I was just bringing this up for discussion, since it's related to
your/Rob's new proposal. I'll send a proper (and tested) patch, along
with a doc update, if that looks reasonable.

Brian

Re: [PATCH 1/3] mtd: create a partition type device tree binding

[Geert Uytterhoeven]

On Fri, Nov 6, 2015 at 3:13 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Oct 30, 2015 at 12:51 PM, Brian Norris
> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Fri, Oct 30, 2015 at 03:00:57PM +0100, Linus Walleij wrote:
>>> On Thu, Oct 29, 2015 at 5:29 PM, Brian Norris
>>> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>> >> +Required properties:
>>> >> +- partition-type : the type of partition. Only one type can be specified.
>>> >
>>> > You're supporting this as a list property (for future expansion,
>>> > presumably), so I can only assume the "only one type" is referring to
>>> > the number of different parsers available currently, not the behavior of
>>> > the property itself?
>>>
>>> I was thinking that it would not be a list actually.

Why not? It is (was) not uncommon to have multiple partition table types on
hard disks (e.g. bsd disklabel and msdos and/or amiga rdb).

>>> The reason being that if you're anyways going to the trouble of
>>> specifying exactly what partition type is going to be used, you're
>>> not really interested in specifying a few different ones, you know
>>> exactly what type it is going to be.
>>
>> OK, that makes sense. I think it's still *possible* that a board might
>> have the option of more than one partition parser, and so they might
>> just include both in the DTS, but that seems unlikely and so it makes
>> sense not to (over)engineer for it before it's needed. Anyway, your
>> binding can easily be expanded in the future if needed.
>
> Since we now have partitions contained in a sub node, how about using
> compatible for that sub node instead.

And "compatible" supports a list of multiple values.

BTW, this means it also (can) becomes more generic. Will it be applicable
to other block devices (e.g. hard disks), too?
Integration with block/partitions?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds