| Message ID | 1449194529-145705-2-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Dec 03, 2015 at 06:02:09PM -0800, Brian Norris wrote: > As noted here [1], there are potentially future conflicts if we try to > use MTD's "partitions" subnode to describe anything besides just the > fixed-in-the-device-tree partitions currently described in this > document. Particularly, there was a proposal to use this node for the > AFS parser too. > > It can pose a (small) problem to try to differentiate the following > nodes: > > // using binding as currently specified > partitions { > #address-cells = <x>; > #size-cells = <y>; > partition@0 { > ...; > }; > }; > > and > > // proposed future binding > partitions { > compatible = "arm,arm-flash-structure"; > }; > > It's especially difficult if other uses of this node start having > subnodes. > > So, since the "partitions" node is new in v4.4, let's fixup the binding > before release so that it requires a compatible property, so it's much > clearer to distinguish. e.g.: > > // proposed > partitions { > compatible = "partitions"; > #address-cells = <x>; > #size-cells = <y>; > partition@0 { > ...; > }; > }; > > [1] Subject: "mtd: create a partition type device tree binding" > http://lkml.kernel.org/g/20151113220039.GA74382@google.com > http://lists.infradead.org/pipermail/linux-mtd/2015-November/063355.html > http://lists.infradead.org/pipermail/linux-mtd/2015-November/063364.html > > Cc: Michal Suchanek <hramrach@gmail.com> > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > This one is more of a future proofing patch. We should probably take this for > 4.4, before the binding "stabilizes" (I don't actually see any users yet), or > else we'll have to find some other (possibly more complicated) way to avoid > this potential collision on future development. Acked-by: Rob Herring <robh@kernel.org> > > Documentation/devicetree/bindings/mtd/partition.txt | 7 ++++++- > drivers/mtd/ofpart.c | 3 +++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt > index f1e2a02381a4..047e80575881 100644 > --- a/Documentation/devicetree/bindings/mtd/partition.txt > +++ b/Documentation/devicetree/bindings/mtd/partition.txt > @@ -6,7 +6,9 @@ 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 > -'partitions'. Partitions are defined in subnodes of the partitions node. > +'partitions'. This node should have the following property: > +- compatible : (required) must be "partitions" > +Partitions are then defined in subnodes of the partitions node. > > For backwards compatibility partitions as direct subnodes of the mtd device are > supported. This use is discouraged. > @@ -36,6 +38,7 @@ Examples: > > flash@0 { > partitions { > + compatible = "partitions"; > #address-cells = <1>; > #size-cells = <1>; > > @@ -53,6 +56,7 @@ flash@0 { > > flash@1 { > partitions { > + compatible = "partitions"; > #address-cells = <1>; > #size-cells = <2>; > > @@ -66,6 +70,7 @@ flash@1 { > > flash@2 { > partitions { > + compatible = "partitions"; > #address-cells = <2>; > #size-cells = <2>; > > diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c > index 3e9c5857c991..23cd809fad4c 100644 > --- a/drivers/mtd/ofpart.c > +++ b/drivers/mtd/ofpart.c > @@ -55,6 +55,9 @@ static int parse_ofpart_partitions(struct mtd_info *master, > master->name, mtd_node->full_name); > ofpart_node = mtd_node; > dedicated = false; > + } else if (!of_device_is_compatible(ofpart_node, "partitions")) { > + /* The 'partitions' subnode might be used by another parser */ > + return 0; > } > > /* First count the subnodes */ > -- > 2.6.0.rc2.230.g3dd15c0 >
On Fri, Dec 4, 2015 at 3:02 AM, Brian Norris <computersforpeace@gmail.com> wrote: > As noted here [1], there are potentially future conflicts if we try to > use MTD's "partitions" subnode to describe anything besides just the > fixed-in-the-device-tree partitions currently described in this > document. Particularly, there was a proposal to use this node for the > AFS parser too. > > It can pose a (small) problem to try to differentiate the following > nodes: > > // using binding as currently specified > partitions { > #address-cells = <x>; > #size-cells = <y>; > partition@0 { > ...; > }; > }; > > and > > // proposed future binding > partitions { > compatible = "arm,arm-flash-structure"; > }; > > It's especially difficult if other uses of this node start having > subnodes. > > So, since the "partitions" node is new in v4.4, let's fixup the binding > before release so that it requires a compatible property, so it's much > clearer to distinguish. e.g.: > > // proposed > partitions { > compatible = "partitions"; "partitions" sounds mode like a device_type thing than a compatible name, maybe "fixed-partitions"? IMHO that would describe better what these are, and doesn't invite to think using compatible = "arm,arm-flash-structure", "partitions"; is a good idea. > #address-cells = <x>; > #size-cells = <y>; > partition@0 { > ...; > }; > }; Jonas
On Sat, Dec 05, 2015 at 12:45:36PM +0100, Jonas Gorski wrote: > On Fri, Dec 4, 2015 at 3:02 AM, Brian Norris > <computersforpeace@gmail.com> wrote: > > // proposed > > partitions { > > compatible = "partitions"; > > "partitions" sounds mode like a device_type thing than a compatible > name, maybe "fixed-partitions"? IMHO that would describe better what > these are, and doesn't invite to think using compatible = > "arm,arm-flash-structure", "partitions"; is a good idea. "fixed-partitions" sounds OK to me. If no objections, I'll apply these patches, with (approximately) a: s/"partitions"/"fixed-partitions"/ Thanks, Brian > > #address-cells = <x>; > > #size-cells = <y>; > > partition@0 { > > ...; > > }; > > };
On Mon, Dec 07, 2015 at 09:58:35AM -0800, Brian Norris wrote: > On Sat, Dec 05, 2015 at 12:45:36PM +0100, Jonas Gorski wrote: > > On Fri, Dec 4, 2015 at 3:02 AM, Brian Norris > > <computersforpeace@gmail.com> wrote: > > > // proposed > > > partitions { > > > compatible = "partitions"; > > > > "partitions" sounds mode like a device_type thing than a compatible > > name, maybe "fixed-partitions"? IMHO that would describe better what > > these are, and doesn't invite to think using compatible = > > "arm,arm-flash-structure", "partitions"; is a good idea. > > "fixed-partitions" sounds OK to me. If no objections, I'll apply these > patches, with (approximately) a: > > s/"partitions"/"fixed-partitions"/ Pushed to linux-mtd.git with the above change.
Hi Brian, On Wed, Dec 9, 2015 at 2:12 AM, Brian Norris <computersforpeace@gmail.com> wrote: > On Mon, Dec 07, 2015 at 09:58:35AM -0800, Brian Norris wrote: >> On Sat, Dec 05, 2015 at 12:45:36PM +0100, Jonas Gorski wrote: >> > On Fri, Dec 4, 2015 at 3:02 AM, Brian Norris >> > <computersforpeace@gmail.com> wrote: >> > > // proposed >> > > partitions { >> > > compatible = "partitions"; >> > >> > "partitions" sounds mode like a device_type thing than a compatible >> > name, maybe "fixed-partitions"? IMHO that would describe better what >> > these are, and doesn't invite to think using compatible = >> > "arm,arm-flash-structure", "partitions"; is a good idea. >> >> "fixed-partitions" sounds OK to me. If no objections, I'll apply these >> patches, with (approximately) a: >> >> s/"partitions"/"fixed-partitions"/ > > Pushed to linux-mtd.git with the above change. Aarghl, hadn't seen this patch before. This breaks the users that have already added the partitions subnodes (armada-xp-lenovo-ix4-300d.dts and a few shmobile). Will send patches to fix it... 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
On Mon, Dec 14, 2015 at 03:49:14PM +0100, Geert Uytterhoeven wrote: > On Wed, Dec 9, 2015 at 2:12 AM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Mon, Dec 07, 2015 at 09:58:35AM -0800, Brian Norris wrote: > >> On Sat, Dec 05, 2015 at 12:45:36PM +0100, Jonas Gorski wrote: > >> > On Fri, Dec 4, 2015 at 3:02 AM, Brian Norris > >> > <computersforpeace@gmail.com> wrote: > >> > > // proposed > >> > > partitions { > >> > > compatible = "partitions"; > >> > > >> > "partitions" sounds mode like a device_type thing than a compatible > >> > name, maybe "fixed-partitions"? IMHO that would describe better what > >> > these are, and doesn't invite to think using compatible = > >> > "arm,arm-flash-structure", "partitions"; is a good idea. > >> > >> "fixed-partitions" sounds OK to me. If no objections, I'll apply these > >> patches, with (approximately) a: > >> > >> s/"partitions"/"fixed-partitions"/ > > > > Pushed to linux-mtd.git with the above change. > > Aarghl, hadn't seen this patch before. > > This breaks the users that have already added the partitions subnodes > (armada-xp-lenovo-ix4-300d.dts and a few shmobile). Sorry, I checked Linus' master and not linux-next :( > Will send patches to fix it... Thanks. Brian
diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt index f1e2a02381a4..047e80575881 100644 --- a/Documentation/devicetree/bindings/mtd/partition.txt +++ b/Documentation/devicetree/bindings/mtd/partition.txt @@ -6,7 +6,9 @@ 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 -'partitions'. Partitions are defined in subnodes of the partitions node. +'partitions'. This node should have the following property: +- compatible : (required) must be "partitions" +Partitions are then defined in subnodes of the partitions node. For backwards compatibility partitions as direct subnodes of the mtd device are supported. This use is discouraged. @@ -36,6 +38,7 @@ Examples: flash@0 { partitions { + compatible = "partitions"; #address-cells = <1>; #size-cells = <1>; @@ -53,6 +56,7 @@ flash@0 { flash@1 { partitions { + compatible = "partitions"; #address-cells = <1>; #size-cells = <2>; @@ -66,6 +70,7 @@ flash@1 { flash@2 { partitions { + compatible = "partitions"; #address-cells = <2>; #size-cells = <2>; diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c index 3e9c5857c991..23cd809fad4c 100644 --- a/drivers/mtd/ofpart.c +++ b/drivers/mtd/ofpart.c @@ -55,6 +55,9 @@ static int parse_ofpart_partitions(struct mtd_info *master, master->name, mtd_node->full_name); ofpart_node = mtd_node; dedicated = false; + } else if (!of_device_is_compatible(ofpart_node, "partitions")) { + /* The 'partitions' subnode might be used by another parser */ + return 0; } /* First count the subnodes */
As noted here [1], there are potentially future conflicts if we try to use MTD's "partitions" subnode to describe anything besides just the fixed-in-the-device-tree partitions currently described in this document. Particularly, there was a proposal to use this node for the AFS parser too. It can pose a (small) problem to try to differentiate the following nodes: // using binding as currently specified partitions { #address-cells = <x>; #size-cells = <y>; partition@0 { ...; }; }; and // proposed future binding partitions { compatible = "arm,arm-flash-structure"; }; It's especially difficult if other uses of this node start having subnodes. So, since the "partitions" node is new in v4.4, let's fixup the binding before release so that it requires a compatible property, so it's much clearer to distinguish. e.g.: // proposed partitions { compatible = "partitions"; #address-cells = <x>; #size-cells = <y>; partition@0 { ...; }; }; [1] Subject: "mtd: create a partition type device tree binding" http://lkml.kernel.org/g/20151113220039.GA74382@google.com http://lists.infradead.org/pipermail/linux-mtd/2015-November/063355.html http://lists.infradead.org/pipermail/linux-mtd/2015-November/063364.html Cc: Michal Suchanek <hramrach@gmail.com> Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- This one is more of a future proofing patch. We should probably take this for 4.4, before the binding "stabilizes" (I don't actually see any users yet), or else we'll have to find some other (possibly more complicated) way to avoid this potential collision on future development. Documentation/devicetree/bindings/mtd/partition.txt | 7 ++++++- drivers/mtd/ofpart.c | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-)