| Message ID | e47db0c8eeaeb11353bcb2f4dcd138e813c3ecd7.1439911625.git.hramrach@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi DT maintainers, It's a bit hypocritical of me, since I've been a slow reviewer as well, but... can we get some review on this one? Usually, I'm comfortable taking driver DT bindings without your review, but this one is a bit more generic and is more far-reaching than the average driver. I'm not a big fan of this change, and I don't quite understand why the bus driver (the SPI bus, which is a level up from the SPI device / MTD node) can specify its grandchildren (see spi-samsung.txt). But given the constraints, I think Michal's solution is OK. And I do agree that MTD's ofpart should be bit more specific. Anyway, a quick look and an Ack/Nak would be appreciated. Thanks, Brian On Tue, Aug 18, 2015 at 03:34:08PM -0000, Michal Suchanek wrote: > To avoid conflict with other drivers using subnodes of the mtd device > create only one ofpart-specific node rather than any number of > arbitrary partition subnodes. > > Signed-off-by: Michal Suchanek <hramrach@gmail.com> > --- > v3: > > - rename DT node ofpart -> partitions > --- > .../devicetree/bindings/mtd/partition.txt | 68 +++++++++++++--------- > 1 file changed, 40 insertions(+), 28 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt > index 8e5557d..8c2aff7 100644 > --- a/Documentation/devicetree/bindings/mtd/partition.txt > +++ b/Documentation/devicetree/bindings/mtd/partition.txt > @@ -4,10 +4,16 @@ 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. > + > +The partition table should be partitions subnode of the mtd node. Partitions are > +defined in subnodes of the partitions node. > + > +For backwards compatibility partitions as direct subnodes of the mtd device are > +supported. This use is discouraged. > 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 > -two valid values for both: > +#address-cells & #size-cells must both be present in the partitions subnode of the > +mtd 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 > @@ -28,44 +34,50 @@ Examples: > > > flash@0 { > - #address-cells = <1>; > - #size-cells = <1>; > + partitions { > + #address-cells = <1>; > + #size-cells = <1>; > > - partition@0 { > - label = "u-boot"; > - reg = <0x0000000 0x100000>; > - read-only; > - }; > + partition@0 { > + label = "u-boot"; > + reg = <0x0000000 0x100000>; > + read-only; > + }; > > - uimage@100000 { > - reg = <0x0100000 0x200000>; > + uimage@100000 { > + reg = <0x0100000 0x200000>; > + }; > }; > }; > > flash@1 { > - #address-cells = <1>; > - #size-cells = <2>; > + partitions { > + #address-cells = <1>; > + #size-cells = <2>; > > - /* a 4 GiB partition */ > - partition@0 { > - label = "filesystem"; > - reg = <0x00000000 0x1 0x00000000>; > + /* a 4 GiB partition */ > + partition@0 { > + label = "filesystem"; > + reg = <0x00000000 0x1 0x00000000>; > + }; > }; > }; > > flash@2 { > - #address-cells = <2>; > - #size-cells = <2>; > + partitions { > + #address-cells = <2>; > + #size-cells = <2>; > > - /* an 8 GiB partition */ > - partition@0 { > - label = "filesystem #1"; > - reg = <0x0 0x00000000 0x2 0x00000000>; > - }; > + /* an 8 GiB partition */ > + partition@0 { > + label = "filesystem #1"; > + reg = <0x0 0x00000000 0x2 0x00000000>; > + }; > > - /* a 4 GiB partition */ > - partition@200000000 { > - label = "filesystem #2"; > - reg = <0x2 0x00000000 0x1 0x00000000>; > + /* a 4 GiB partition */ > + partition@200000000 { > + label = "filesystem #2"; > + reg = <0x2 0x00000000 0x1 0x00000000>; > + }; > }; > }; > -- > 2.1.4 >
On Sun, Oct 11, 2015 at 01:04:02PM -0700, Brian Norris wrote: > Hi DT maintainers, Last call: I'd like a reviewer that (1) does DT reviews somewhat regularly and (2) is not me. If I can't get one soon, I'll take this for 4.4. Thanks, Brian > It's a bit hypocritical of me, since I've been a slow reviewer as well, > but... can we get some review on this one? Usually, I'm comfortable > taking driver DT bindings without your review, but this one is a bit > more generic and is more far-reaching than the average driver. > > I'm not a big fan of this change, and I don't quite understand why the > bus driver (the SPI bus, which is a level up from the SPI device / MTD > node) can specify its grandchildren (see spi-samsung.txt). But given the > constraints, I think Michal's solution is OK. And I do agree that MTD's > ofpart should be bit more specific. > > Anyway, a quick look and an Ack/Nak would be appreciated. > > Thanks, > Brian > > On Tue, Aug 18, 2015 at 03:34:08PM -0000, Michal Suchanek wrote: > > To avoid conflict with other drivers using subnodes of the mtd device > > create only one ofpart-specific node rather than any number of > > arbitrary partition subnodes. > > > > Signed-off-by: Michal Suchanek <hramrach@gmail.com> > > --- > > v3: > > > > - rename DT node ofpart -> partitions > > --- > > .../devicetree/bindings/mtd/partition.txt | 68 +++++++++++++--------- > > 1 file changed, 40 insertions(+), 28 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt > > index 8e5557d..8c2aff7 100644 > > --- a/Documentation/devicetree/bindings/mtd/partition.txt > > +++ b/Documentation/devicetree/bindings/mtd/partition.txt > > @@ -4,10 +4,16 @@ 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. > > + > > +The partition table should be partitions subnode of the mtd node. Partitions are > > +defined in subnodes of the partitions node. > > + > > +For backwards compatibility partitions as direct subnodes of the mtd device are > > +supported. This use is discouraged. > > 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 > > -two valid values for both: > > +#address-cells & #size-cells must both be present in the partitions subnode of the > > +mtd 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 > > @@ -28,44 +34,50 @@ Examples: > > > > > > flash@0 { > > - #address-cells = <1>; > > - #size-cells = <1>; > > + partitions { > > + #address-cells = <1>; > > + #size-cells = <1>; > > > > - partition@0 { > > - label = "u-boot"; > > - reg = <0x0000000 0x100000>; > > - read-only; > > - }; > > + partition@0 { > > + label = "u-boot"; > > + reg = <0x0000000 0x100000>; > > + read-only; > > + }; > > > > - uimage@100000 { > > - reg = <0x0100000 0x200000>; > > + uimage@100000 { > > + reg = <0x0100000 0x200000>; > > + }; > > }; > > }; > > > > flash@1 { > > - #address-cells = <1>; > > - #size-cells = <2>; > > + partitions { > > + #address-cells = <1>; > > + #size-cells = <2>; > > > > - /* a 4 GiB partition */ > > - partition@0 { > > - label = "filesystem"; > > - reg = <0x00000000 0x1 0x00000000>; > > + /* a 4 GiB partition */ > > + partition@0 { > > + label = "filesystem"; > > + reg = <0x00000000 0x1 0x00000000>; > > + }; > > }; > > }; > > > > flash@2 { > > - #address-cells = <2>; > > - #size-cells = <2>; > > + partitions { > > + #address-cells = <2>; > > + #size-cells = <2>; > > > > - /* an 8 GiB partition */ > > - partition@0 { > > - label = "filesystem #1"; > > - reg = <0x0 0x00000000 0x2 0x00000000>; > > - }; > > + /* an 8 GiB partition */ > > + partition@0 { > > + label = "filesystem #1"; > > + reg = <0x0 0x00000000 0x2 0x00000000>; > > + }; > > > > - /* a 4 GiB partition */ > > - partition@200000000 { > > - label = "filesystem #2"; > > - reg = <0x2 0x00000000 0x1 0x00000000>; > > + /* a 4 GiB partition */ > > + partition@200000000 { > > + label = "filesystem #2"; > > + reg = <0x2 0x00000000 0x1 0x00000000>; > > + }; > > }; > > }; > > -- > > 2.1.4 > >
On Sun, Oct 11, 2015 at 3:04 PM, Brian Norris <computersforpeace@gmail.com> wrote: > Hi DT maintainers, > > It's a bit hypocritical of me, since I've been a slow reviewer as well, > but... can we get some review on this one? Usually, I'm comfortable > taking driver DT bindings without your review, but this one is a bit > more generic and is more far-reaching than the average driver. Sorry, missed this one. This would be a good one to send to devicetree-spec to BTW. > I'm not a big fan of this change, and I don't quite understand why the > bus driver (the SPI bus, which is a level up from the SPI device / MTD > node) can specify its grandchildren (see spi-samsung.txt). But given the That's just an example. I just would change it. > constraints, I think Michal's solution is OK. And I do agree that MTD's > ofpart should be bit more specific. > > Anyway, a quick look and an Ack/Nak would be appreciated. Looks fine to me: Acked-by: Rob Herring <robh@kernel.org> > > Thanks, > Brian > > On Tue, Aug 18, 2015 at 03:34:08PM -0000, Michal Suchanek wrote: >> To avoid conflict with other drivers using subnodes of the mtd device >> create only one ofpart-specific node rather than any number of >> arbitrary partition subnodes. >> >> Signed-off-by: Michal Suchanek <hramrach@gmail.com> >> --- >> v3: >> >> - rename DT node ofpart -> partitions >> --- >> .../devicetree/bindings/mtd/partition.txt | 68 +++++++++++++--------- >> 1 file changed, 40 insertions(+), 28 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt >> index 8e5557d..8c2aff7 100644 >> --- a/Documentation/devicetree/bindings/mtd/partition.txt >> +++ b/Documentation/devicetree/bindings/mtd/partition.txt >> @@ -4,10 +4,16 @@ 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. >> + >> +The partition table should be partitions subnode of the mtd node. Partitions are >> +defined in subnodes of the partitions node. >> + >> +For backwards compatibility partitions as direct subnodes of the mtd device are >> +supported. This use is discouraged. >> 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 >> -two valid values for both: >> +#address-cells & #size-cells must both be present in the partitions subnode of the >> +mtd 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 >> @@ -28,44 +34,50 @@ Examples: >> >> >> flash@0 { >> - #address-cells = <1>; >> - #size-cells = <1>; >> + partitions { >> + #address-cells = <1>; >> + #size-cells = <1>; >> >> - partition@0 { >> - label = "u-boot"; >> - reg = <0x0000000 0x100000>; >> - read-only; >> - }; >> + partition@0 { >> + label = "u-boot"; >> + reg = <0x0000000 0x100000>; >> + read-only; >> + }; >> >> - uimage@100000 { >> - reg = <0x0100000 0x200000>; >> + uimage@100000 { >> + reg = <0x0100000 0x200000>; >> + }; >> }; >> }; >> >> flash@1 { >> - #address-cells = <1>; >> - #size-cells = <2>; >> + partitions { >> + #address-cells = <1>; >> + #size-cells = <2>; >> >> - /* a 4 GiB partition */ >> - partition@0 { >> - label = "filesystem"; >> - reg = <0x00000000 0x1 0x00000000>; >> + /* a 4 GiB partition */ >> + partition@0 { >> + label = "filesystem"; >> + reg = <0x00000000 0x1 0x00000000>; >> + }; >> }; >> }; >> >> flash@2 { >> - #address-cells = <2>; >> - #size-cells = <2>; >> + partitions { >> + #address-cells = <2>; >> + #size-cells = <2>; >> >> - /* an 8 GiB partition */ >> - partition@0 { >> - label = "filesystem #1"; >> - reg = <0x0 0x00000000 0x2 0x00000000>; >> - }; >> + /* an 8 GiB partition */ >> + partition@0 { >> + label = "filesystem #1"; >> + reg = <0x0 0x00000000 0x2 0x00000000>; >> + }; >> >> - /* a 4 GiB partition */ >> - partition@200000000 { >> - label = "filesystem #2"; >> - reg = <0x2 0x00000000 0x1 0x00000000>; >> + /* a 4 GiB partition */ >> + partition@200000000 { >> + label = "filesystem #2"; >> + reg = <0x2 0x00000000 0x1 0x00000000>; >> + }; >> }; >> }; >> -- >> 2.1.4 >>
Hi Rob, Thanks for the review. On Mon, Oct 26, 2015 at 11:35:24PM -0500, Rob Herring wrote: > On Sun, Oct 11, 2015 at 3:04 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > Hi DT maintainers, > > > > It's a bit hypocritical of me, since I've been a slow reviewer as well, > > but... can we get some review on this one? Usually, I'm comfortable > > taking driver DT bindings without your review, but this one is a bit > > more generic and is more far-reaching than the average driver. > > Sorry, missed this one. This would be a good one to send to > devicetree-spec to BTW. I'm not very familiar with that list. With the intention of getting into an ePAPR (or similar) spec? Or just for additional review? If the former, would you suggest codifying both the old and the new, or just the new? Brian
On Tue, Oct 27, 2015 at 5:50 PM, Brian Norris <computersforpeace@gmail.com> wrote: > Hi Rob, > > Thanks for the review. > > On Mon, Oct 26, 2015 at 11:35:24PM -0500, Rob Herring wrote: >> On Sun, Oct 11, 2015 at 3:04 PM, Brian Norris >> <computersforpeace@gmail.com> wrote: >> > Hi DT maintainers, >> > >> > It's a bit hypocritical of me, since I've been a slow reviewer as well, >> > but... can we get some review on this one? Usually, I'm comfortable >> > taking driver DT bindings without your review, but this one is a bit >> > more generic and is more far-reaching than the average driver. >> >> Sorry, missed this one. This would be a good one to send to >> devicetree-spec to BTW. > > I'm not very familiar with that list. With the intention of getting into > an ePAPR (or similar) spec? Or just for additional review? If the > former, would you suggest codifying both the old and the new, or just > the new? I would say it is for anything not driver specific. It was created to separate out the driver binding firehose from the common bindings and get more non-Linux participation on those. Perhaps it was poorly named. I want to improve the split in docs so the appropriate list is used. Sending to both devicetree and devicetree-spec is fine. Rob
diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt index 8e5557d..8c2aff7 100644 --- a/Documentation/devicetree/bindings/mtd/partition.txt +++ b/Documentation/devicetree/bindings/mtd/partition.txt @@ -4,10 +4,16 @@ 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. + +The partition table should be partitions subnode of the mtd node. Partitions are +defined in subnodes of the partitions node. + +For backwards compatibility partitions as direct subnodes of the mtd device are +supported. This use is discouraged. 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 -two valid values for both: +#address-cells & #size-cells must both be present in the partitions subnode of the +mtd 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 @@ -28,44 +34,50 @@ Examples: flash@0 { - #address-cells = <1>; - #size-cells = <1>; + partitions { + #address-cells = <1>; + #size-cells = <1>; - partition@0 { - label = "u-boot"; - reg = <0x0000000 0x100000>; - read-only; - }; + partition@0 { + label = "u-boot"; + reg = <0x0000000 0x100000>; + read-only; + }; - uimage@100000 { - reg = <0x0100000 0x200000>; + uimage@100000 { + reg = <0x0100000 0x200000>; + }; }; }; flash@1 { - #address-cells = <1>; - #size-cells = <2>; + partitions { + #address-cells = <1>; + #size-cells = <2>; - /* a 4 GiB partition */ - partition@0 { - label = "filesystem"; - reg = <0x00000000 0x1 0x00000000>; + /* a 4 GiB partition */ + partition@0 { + label = "filesystem"; + reg = <0x00000000 0x1 0x00000000>; + }; }; }; flash@2 { - #address-cells = <2>; - #size-cells = <2>; + partitions { + #address-cells = <2>; + #size-cells = <2>; - /* an 8 GiB partition */ - partition@0 { - label = "filesystem #1"; - reg = <0x0 0x00000000 0x2 0x00000000>; - }; + /* an 8 GiB partition */ + partition@0 { + label = "filesystem #1"; + reg = <0x0 0x00000000 0x2 0x00000000>; + }; - /* a 4 GiB partition */ - partition@200000000 { - label = "filesystem #2"; - reg = <0x2 0x00000000 0x1 0x00000000>; + /* a 4 GiB partition */ + partition@200000000 { + label = "filesystem #2"; + reg = <0x2 0x00000000 0x1 0x00000000>; + }; }; };
To avoid conflict with other drivers using subnodes of the mtd device create only one ofpart-specific node rather than any number of arbitrary partition subnodes. Signed-off-by: Michal Suchanek <hramrach@gmail.com> --- v3: - rename DT node ofpart -> partitions --- .../devicetree/bindings/mtd/partition.txt | 68 +++++++++++++--------- 1 file changed, 40 insertions(+), 28 deletions(-)