| Message ID | 1446123152-22666-1-git-send-email-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+ 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@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@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 >
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
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
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
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
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
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/
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
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@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
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
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@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(-)