linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] mtd: create a partition type device tree binding
@ 2015-10-29 12:52 Linus Walleij
  2015-10-29 12:52 ` [PATCH 2/3] mtd: physmap_of: break out array clone code Linus Walleij
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Linus Walleij @ 2015-10-29 12:52 UTC (permalink / raw)
  To: linux-arm-kernel

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 at vger.kernel.org
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.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 at 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] mtd: physmap_of: break out array clone code
  2015-10-29 12:52 [PATCH 1/3] mtd: create a partition type device tree binding Linus Walleij
@ 2015-10-29 12:52 ` Linus Walleij
  2015-10-29 12:52 ` [PATCH 3/3] mtd: physmap_of: handle the new "partition-type" Linus Walleij
  2015-10-29 16:29 ` [PATCH 1/3] mtd: create a partition type device tree binding Brian Norris
  2 siblings, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2015-10-29 12:52 UTC (permalink / raw)
  To: linux-arm-kernel

There is a piece of code cloning a string array in the end of
the of_get_probes() function. Break it out, so we can reuse it.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/maps/physmap_of.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 3e614e9119d5..1b66ca66206b 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -112,17 +112,18 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev,
 static const char * const part_probe_types_def[] = {
 	"cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
 
-static const char * const *of_get_probes(struct device_node *dp)
+/**
+ * string_to_array() - split a string into an array
+ *
+ * This takes "a\0string\0array" and splits it into { "a", "string", "array" }
+ * with a newly allocated array of pointers returned. A NULL terminator
+ * is also added at the end of the array.
+ */
+static const char * const *string_to_array(const char *cp, int cplen)
 {
-	const char *cp;
-	int cplen;
+	const char **res;
 	unsigned int l;
 	unsigned int count;
-	const char **res;
-
-	cp = of_get_property(dp, "linux,part-probe", &cplen);
-	if (cp == NULL)
-		return part_probe_types_def;
 
 	count = 0;
 	for (l = 0; l != cplen; l++)
@@ -143,6 +144,18 @@ static const char * const *of_get_probes(struct device_node *dp)
 	return res;
 }
 
+static const char * const *of_get_probes(struct device_node *dp)
+{
+	const char *cp;
+	int cplen;
+	int ret;
+
+	cp = of_get_property(dp, "linux,part-probe", &cplen);
+	if (cp == NULL)
+		return part_probe_types_def;
+	return string_to_array(cp, cplen);
+}
+
 static void of_free_probes(const char * const *probes)
 {
 	if (probes != part_probe_types_def)
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] mtd: physmap_of: handle the new "partition-type"
  2015-10-29 12:52 [PATCH 1/3] mtd: create a partition type device tree binding Linus Walleij
  2015-10-29 12:52 ` [PATCH 2/3] mtd: physmap_of: break out array clone code Linus Walleij
@ 2015-10-29 12:52 ` Linus Walleij
  2015-10-29 16:29 ` [PATCH 1/3] mtd: create a partition type device tree binding Brian Norris
  2 siblings, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2015-10-29 12:52 UTC (permalink / raw)
  To: linux-arm-kernel

We defined a new device tree binding for partitions, and if they
are found these need to be translated into a string array of
Linux partitions. Reuse the string-to-array split function and
support "arm,arm-flash-structure" by splitting "afs" into
{"afs", NULL} and passing that along.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/maps/physmap_of.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 1b66ca66206b..8c09d7e23b6e 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -105,7 +105,7 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev,
 	}
 }
 
-/* When partitions are set we look for a linux,part-probe property which
+/* When partitions are set we look for a partition-type property which
    specifies the list of partition probers to use. If none is given then the
    default is use. These take precedence over other device tree
    information. */
@@ -150,9 +150,25 @@ static const char * const *of_get_probes(struct device_node *dp)
 	int cplen;
 	int ret;
 
+	/*
+	 * The proper device tree bindings take precedence.
+	 */
+	ret = of_property_read_string(dp, "partition-type", &cp);
+	if (!ret) {
+		pr_info("check for partition type \"%s\"\n", cp);
+		if (!strcmp(cp, "arm,arm-flash-structure"))
+			return string_to_array("afs", 1);
+	}
+
+	/*
+	 * Else look for the deprecated binding, or fall back to
+	 * defaults.
+	 */
 	cp = of_get_property(dp, "linux,part-probe", &cplen);
 	if (cp == NULL)
 		return part_probe_types_def;
+	pr_info("DT is using the deprecated \"linux,part-probe\" binding\n");
+	pr_info("please move to the standard \"partition-type\" binding\n");
 	return string_to_array(cp, cplen);
 }
 
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-10-29 12:52 [PATCH 1/3] mtd: create a partition type device tree binding Linus Walleij
  2015-10-29 12:52 ` [PATCH 2/3] mtd: physmap_of: break out array clone code Linus Walleij
  2015-10-29 12:52 ` [PATCH 3/3] mtd: physmap_of: handle the new "partition-type" Linus Walleij
@ 2015-10-29 16:29 ` Brian Norris
  2015-10-30 14:00   ` Linus Walleij
  2 siblings, 1 reply; 13+ messages in thread
From: Brian Norris @ 2015-10-29 16:29 UTC (permalink / raw)
  To: linux-arm-kernel

+ 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 at vger.kernel.org
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.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 at 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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-10-29 16:29 ` [PATCH 1/3] mtd: create a partition type device tree binding Brian Norris
@ 2015-10-30 14:00   ` Linus Walleij
  2015-10-30 17:51     ` Brian Norris
  0 siblings, 1 reply; 13+ messages in thread
From: Linus Walleij @ 2015-10-30 14:00 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Oct 29, 2015 at 5:29 PM, Brian Norris
<computersforpeace@gmail.com> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-10-30 14:00   ` Linus Walleij
@ 2015-10-30 17:51     ` Brian Norris
  2015-11-06 14:13       ` Rob Herring
  0 siblings, 1 reply; 13+ messages in thread
From: Brian Norris @ 2015-10-30 17:51 UTC (permalink / raw)
  To: linux-arm-kernel

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@gmail.com> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-10-30 17:51     ` Brian Norris
@ 2015-11-06 14:13       ` Rob Herring
  2015-11-10  3:26         ` Brian Norris
                           ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Rob Herring @ 2015-11-06 14:13 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Oct 30, 2015 at 12:51 PM, Brian Norris
<computersforpeace@gmail.com> 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@gmail.com> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-11-06 14:13       ` Rob Herring
@ 2015-11-10  3:26         ` Brian Norris
  2015-11-10  8:43           ` Linus Walleij
  2015-11-13 22:00         ` Brian Norris
  2015-11-15  9:06         ` Geert Uytterhoeven
  2 siblings, 1 reply; 13+ messages in thread
From: Brian Norris @ 2015-11-10  3:26 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-11-10  3:26         ` Brian Norris
@ 2015-11-10  8:43           ` Linus Walleij
  0 siblings, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2015-11-10  8:43 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 10, 2015 at 4:26 AM, Brian Norris
<computersforpeace@gmail.com> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-11-06 14:13       ` Rob Herring
  2015-11-10  3:26         ` Brian Norris
@ 2015-11-13 22:00         ` Brian Norris
  2015-11-14 10:46           ` Linus Walleij
  2015-11-15  9:06         ` Geert Uytterhoeven
  2 siblings, 1 reply; 13+ messages in thread
From: Brian Norris @ 2015-11-13 22:00 UTC (permalink / raw)
  To: linux-arm-kernel

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 at 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/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-11-13 22:00         ` Brian Norris
@ 2015-11-14 10:46           ` Linus Walleij
  2015-11-16  4:12             ` Brian Norris
  0 siblings, 1 reply; 13+ messages in thread
From: Linus Walleij @ 2015-11-14 10:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Nov 13, 2015 at 11:00 PM, Brian Norris
<computersforpeace@gmail.com> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-11-06 14:13       ` Rob Herring
  2015-11-10  3:26         ` Brian Norris
  2015-11-13 22:00         ` Brian Norris
@ 2015-11-15  9:06         ` Geert Uytterhoeven
  2 siblings, 0 replies; 13+ messages in thread
From: Geert Uytterhoeven @ 2015-11-15  9:06 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Nov 6, 2015 at 3:13 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Oct 30, 2015 at 12:51 PM, Brian Norris
> <computersforpeace@gmail.com> 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@gmail.com> 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 at linux-m68k.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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mtd: create a partition type device tree binding
  2015-11-14 10:46           ` Linus Walleij
@ 2015-11-16  4:12             ` Brian Norris
  0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2015-11-16  4:12 UTC (permalink / raw)
  To: linux-arm-kernel

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@gmail.com> 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 at 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-11-16  4:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-29 12:52 [PATCH 1/3] mtd: create a partition type device tree binding Linus Walleij
2015-10-29 12:52 ` [PATCH 2/3] mtd: physmap_of: break out array clone code Linus Walleij
2015-10-29 12:52 ` [PATCH 3/3] mtd: physmap_of: handle the new "partition-type" Linus Walleij
2015-10-29 16:29 ` [PATCH 1/3] mtd: create a partition type device tree binding Brian Norris
2015-10-30 14:00   ` Linus Walleij
2015-10-30 17:51     ` Brian Norris
2015-11-06 14:13       ` Rob Herring
2015-11-10  3:26         ` Brian Norris
2015-11-10  8:43           ` Linus Walleij
2015-11-13 22:00         ` Brian Norris
2015-11-14 10:46           ` Linus Walleij
2015-11-16  4:12             ` Brian Norris
2015-11-15  9:06         ` Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).