| Message ID | 20180523171448.26234-1-zajec5@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-bindings: mtd: explicitly describe nesting partitions | expand |
On Wed, 23 May 2018 19:14:48 +0200 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This supports nested partitions in a DT. If selected partition has a > "compatible" property specified it will be parsed looking for > subpartitions. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > drivers/mtd/mtdpart.c | 33 +++++++++++++-------------------- > 1 file changed, 13 insertions(+), 20 deletions(-) > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index f8d3a015cdad..52e2cb35fc79 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -322,22 +322,6 @@ static inline void free_partition(struct mtd_part *p) > kfree(p); > } > > -/** > - * mtd_parse_part - parse MTD partition looking for subpartitions > - * > - * @slave: part that is supposed to be a container and should be parsed > - * @types: NULL-terminated array with names of partition parsers to try > - * > - * Some partitions are kind of containers with extra subpartitions (volumes). > - * There can be various formats of such containers. This function tries to use > - * specified parsers to analyze given partition and registers found > - * subpartitions on success. > - */ > -static int mtd_parse_part(struct mtd_part *slave, const char *const *types) > -{ > - return parse_mtd_partitions(&slave->mtd, types, NULL); > -} > - > static struct mtd_part *allocate_partition(struct mtd_info *parent, > const struct mtd_partition *part, int partno, > uint64_t cur_offset) > @@ -735,8 +719,8 @@ int add_mtd_partitions(struct mtd_info *master, > > add_mtd_device(&slave->mtd); > mtd_add_partition_attrs(slave); > - if (parts[i].types) > - mtd_parse_part(slave, parts[i].types); > + /* Look for subpartitions */ > + parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); > > cur_offset = slave->offset + slave->mtd.size; > } > @@ -812,6 +796,12 @@ static const char * const default_mtd_part_types[] = { > NULL > }; > > +/* Check DT only when looking for subpartitions. */ > +static const char * const default_subpartition_types[] = { > + "ofpart", > + NULL > +}; > + > static int mtd_part_do_parse(struct mtd_part_parser *parser, > struct mtd_info *master, > struct mtd_partitions *pparts, > @@ -882,7 +872,9 @@ static int mtd_part_of_parse(struct mtd_info *master, > const char *fixed = "fixed-partitions"; > int ret, err = 0; > > - np = of_get_child_by_name(mtd_get_of_node(master), "partitions"); > + np = mtd_get_of_node(master); > + if (!mtd_is_partition(master)) > + np = of_get_child_by_name(np, "partitions"); > of_property_for_each_string(np, "compatible", prop, compat) { > parser = mtd_part_get_compatible_parser(compat); > if (!parser) > @@ -945,7 +937,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > int ret, err = 0; > > if (!types) > - types = default_mtd_part_types; > + types = mtd_is_partition(master) ? default_subpartition_types : > + default_mtd_part_types; Hm, that means the subparts inherit the parser types from their parent if types != NULL? Is that really what we want? And if that's what we want, why don't we do the same for types == NULL? > > for ( ; *types; types++) { > /* -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23.05.2018 20:24, Boris Brezillon wrote: > On Wed, 23 May 2018 19:14:48 +0200 > Rafał Miłecki <zajec5@gmail.com> wrote: > >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This supports nested partitions in a DT. If selected partition has a >> "compatible" property specified it will be parsed looking for >> subpartitions. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> drivers/mtd/mtdpart.c | 33 +++++++++++++-------------------- >> 1 file changed, 13 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> index f8d3a015cdad..52e2cb35fc79 100644 >> --- a/drivers/mtd/mtdpart.c >> +++ b/drivers/mtd/mtdpart.c >> @@ -322,22 +322,6 @@ static inline void free_partition(struct mtd_part *p) >> kfree(p); >> } >> >> -/** >> - * mtd_parse_part - parse MTD partition looking for subpartitions >> - * >> - * @slave: part that is supposed to be a container and should be parsed >> - * @types: NULL-terminated array with names of partition parsers to try >> - * >> - * Some partitions are kind of containers with extra subpartitions (volumes). >> - * There can be various formats of such containers. This function tries to use >> - * specified parsers to analyze given partition and registers found >> - * subpartitions on success. >> - */ >> -static int mtd_parse_part(struct mtd_part *slave, const char *const *types) >> -{ >> - return parse_mtd_partitions(&slave->mtd, types, NULL); >> -} >> - >> static struct mtd_part *allocate_partition(struct mtd_info *parent, >> const struct mtd_partition *part, int partno, >> uint64_t cur_offset) >> @@ -735,8 +719,8 @@ int add_mtd_partitions(struct mtd_info *master, >> >> add_mtd_device(&slave->mtd); >> mtd_add_partition_attrs(slave); >> - if (parts[i].types) >> - mtd_parse_part(slave, parts[i].types); >> + /* Look for subpartitions */ >> + parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); >> >> cur_offset = slave->offset + slave->mtd.size; >> } >> @@ -812,6 +796,12 @@ static const char * const default_mtd_part_types[] = { >> NULL >> }; >> >> +/* Check DT only when looking for subpartitions. */ >> +static const char * const default_subpartition_types[] = { >> + "ofpart", >> + NULL >> +}; >> + >> static int mtd_part_do_parse(struct mtd_part_parser *parser, >> struct mtd_info *master, >> struct mtd_partitions *pparts, >> @@ -882,7 +872,9 @@ static int mtd_part_of_parse(struct mtd_info *master, >> const char *fixed = "fixed-partitions"; >> int ret, err = 0; >> >> - np = of_get_child_by_name(mtd_get_of_node(master), "partitions"); >> + np = mtd_get_of_node(master); >> + if (!mtd_is_partition(master)) >> + np = of_get_child_by_name(np, "partitions"); >> of_property_for_each_string(np, "compatible", prop, compat) { >> parser = mtd_part_get_compatible_parser(compat); >> if (!parser) >> @@ -945,7 +937,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, >> int ret, err = 0; >> >> if (!types) >> - types = default_mtd_part_types; >> + types = mtd_is_partition(master) ? default_subpartition_types : >> + default_mtd_part_types; > > Hm, that means the subparts inherit the parser types from their parent > if types != NULL? Is that really what we want? And if that's what we > want, why don't we do the same for types == NULL? No, unless I'm missing something. In add_mtd_partitions() there is now a following call: parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); In most cases parts[i].types is NULL, unless you're using some exceptional driver (like bcm47xxpart) which sets "types". So for each partition parse_mtd_partitions() will be called with "types" argument almost always set to NULL (no inheriting). That will result in picking default_subpartition_types. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 24 May 2018 07:50:10 +0200 Rafał Miłecki <zajec5@gmail.com> wrote: > On 23.05.2018 20:24, Boris Brezillon wrote: > > On Wed, 23 May 2018 19:14:48 +0200 > > Rafał Miłecki <zajec5@gmail.com> wrote: > > > >> From: Rafał Miłecki <rafal@milecki.pl> > >> > >> This supports nested partitions in a DT. If selected partition has a > >> "compatible" property specified it will be parsed looking for > >> subpartitions. > >> > >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >> --- > >> drivers/mtd/mtdpart.c | 33 +++++++++++++-------------------- > >> 1 file changed, 13 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > >> index f8d3a015cdad..52e2cb35fc79 100644 > >> --- a/drivers/mtd/mtdpart.c > >> +++ b/drivers/mtd/mtdpart.c > >> @@ -322,22 +322,6 @@ static inline void free_partition(struct mtd_part *p) > >> kfree(p); > >> } > >> > >> -/** > >> - * mtd_parse_part - parse MTD partition looking for subpartitions > >> - * > >> - * @slave: part that is supposed to be a container and should be parsed > >> - * @types: NULL-terminated array with names of partition parsers to try > >> - * > >> - * Some partitions are kind of containers with extra subpartitions (volumes). > >> - * There can be various formats of such containers. This function tries to use > >> - * specified parsers to analyze given partition and registers found > >> - * subpartitions on success. > >> - */ > >> -static int mtd_parse_part(struct mtd_part *slave, const char *const *types) > >> -{ > >> - return parse_mtd_partitions(&slave->mtd, types, NULL); > >> -} > >> - > >> static struct mtd_part *allocate_partition(struct mtd_info *parent, > >> const struct mtd_partition *part, int partno, > >> uint64_t cur_offset) > >> @@ -735,8 +719,8 @@ int add_mtd_partitions(struct mtd_info *master, > >> > >> add_mtd_device(&slave->mtd); > >> mtd_add_partition_attrs(slave); > >> - if (parts[i].types) > >> - mtd_parse_part(slave, parts[i].types); > >> + /* Look for subpartitions */ > >> + parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); > >> > >> cur_offset = slave->offset + slave->mtd.size; > >> } > >> @@ -812,6 +796,12 @@ static const char * const default_mtd_part_types[] = { > >> NULL > >> }; > >> > >> +/* Check DT only when looking for subpartitions. */ > >> +static const char * const default_subpartition_types[] = { > >> + "ofpart", > >> + NULL > >> +}; > >> + > >> static int mtd_part_do_parse(struct mtd_part_parser *parser, > >> struct mtd_info *master, > >> struct mtd_partitions *pparts, > >> @@ -882,7 +872,9 @@ static int mtd_part_of_parse(struct mtd_info *master, > >> const char *fixed = "fixed-partitions"; > >> int ret, err = 0; > >> > >> - np = of_get_child_by_name(mtd_get_of_node(master), "partitions"); > >> + np = mtd_get_of_node(master); > >> + if (!mtd_is_partition(master)) > >> + np = of_get_child_by_name(np, "partitions"); > >> of_property_for_each_string(np, "compatible", prop, compat) { > >> parser = mtd_part_get_compatible_parser(compat); > >> if (!parser) > >> @@ -945,7 +937,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > >> int ret, err = 0; > >> > >> if (!types) > >> - types = default_mtd_part_types; > >> + types = mtd_is_partition(master) ? default_subpartition_types : > >> + default_mtd_part_types; > > > > Hm, that means the subparts inherit the parser types from their parent > > if types != NULL? Is that really what we want? And if that's what we > > want, why don't we do the same for types == NULL? > > No, unless I'm missing something. In add_mtd_partitions() there is now > a following call: > parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); > > In most cases parts[i].types is NULL, unless you're using some > exceptional driver (like bcm47xxpart) which sets "types". > > So for each partition parse_mtd_partitions() will be called with "types" > argument almost always set to NULL (no inheriting). That will result in > picking default_subpartition_types. And that's the "almost always" I'm not happy with. Either you don't want to inherit the types from the parent and you do that for both != NULL and == NULL, or you inherit it for both. Also, I'd really like to improve the 'part parser types' selection logic by letting the boards select the part parsers instead of having the MTD drivers do it. Maybe we could have an approach similar to the gpio-lookup-table where board files could assign a part-parser list to an MTD name, and then let the core find whether there's a part parser list attached to the MTD dev at registration time. And for DT-based setup, this information would just be extracted from the compatible of the 'partitions' node (or the node directly if the MTD device is a partition). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 23, 2018 at 07:14:47PM +0200, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Documentation was already saying that fixed and dynamic partitioning can > be mixed but was missing a clear description and examples. This commit > adds a proper description of how partitions can be nested and how layout > descriptions can be mixed. > > This addition is important for partitions that contain subpartitions. > It's useful to have parent partition registered (e.g. for overwriting > purposes) as well as children ones (for accessing data). It's also > required when a single partition uses different partitioning method > (e.g. vendor custom "firmware" partition with kernel + rootfs). > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > Another example (I couldn't put in Documentation yet) could be: > > flash@0 { > partitions { > compatible = "fixed-partitions"; > #address-cells = <1>; > #size-cells = <1>; > > partition@0 { > label = "bootloader"; > reg = <0x0000000 0x0100000>; > read-only; > }; > > firmware@100000 { > label = "firmware"; > reg = <0x0100000 0x0800000>; > compatible = "brcm,trx"; > }; > }; > }; > > It's probably even more realistic one, but we don't describe "brcm,trx" > binding yet. This one makes more sense to me than the one you've added because if you only have fixed partitions, it seems like most times you could just flatten them to 1 level. I suppose having some levels could make doing updates easier. > The purpose of above description would be to: > 1) Specify fixed partitions (they never change) > 2) Tell operating system that "firmware" partition uses Broadcom's TRX > format which is a container for 2 or 3 subpartitions (usually: kernel > and rootfs). > --- > .../devicetree/bindings/mtd/partition.txt | 25 ++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt > index a8f382642ba9..2214167ce88a 100644 > --- a/Documentation/devicetree/bindings/mtd/partition.txt > +++ b/Documentation/devicetree/bindings/mtd/partition.txt > @@ -14,6 +14,13 @@ method is used for a given flash device. To describe the method there should be > a subnode of the flash device that is named 'partitions'. It must have a > 'compatible' property, which is used to identify the method to use. > > +When a single partition is represented with a DT node (it depends on a used > +format) it may also be described using above rules ('compatible' and optionally > +some extra properties / subnodes). It allows describing more complex, > +hierarchical (multi-level) layouts and should be used if there is some > +significant relation between partitions or some partition internally uses > +another partitioning method. > + > Available bindings are listed in the "partitions" subdirectory. > > > @@ -73,6 +80,24 @@ flash@0 { > uimage@100000 { > reg = <0x0100000 0x200000>; > }; > + > + calibration@200000 { > + label = "calibration"; > + reg = <0x0200000 0x100000>; > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; You are missing 'ranges' here. > + > + partition@0 { > + label = "wifi0"; > + reg = <0x000000 0x080000>; > + }; > + > + partition@80000 { > + label = "wifi1"; > + reg = <0x080000 0x080000>; > + }; > + }; > }; > }; > > -- > 2.13.6 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt index a8f382642ba9..2214167ce88a 100644 --- a/Documentation/devicetree/bindings/mtd/partition.txt +++ b/Documentation/devicetree/bindings/mtd/partition.txt @@ -14,6 +14,13 @@ method is used for a given flash device. To describe the method there should be a subnode of the flash device that is named 'partitions'. It must have a 'compatible' property, which is used to identify the method to use. +When a single partition is represented with a DT node (it depends on a used +format) it may also be described using above rules ('compatible' and optionally +some extra properties / subnodes). It allows describing more complex, +hierarchical (multi-level) layouts and should be used if there is some +significant relation between partitions or some partition internally uses +another partitioning method. + Available bindings are listed in the "partitions" subdirectory. @@ -73,6 +80,24 @@ flash@0 { uimage@100000 { reg = <0x0100000 0x200000>; }; + + calibration@200000 { + label = "calibration"; + reg = <0x0200000 0x100000>; + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "wifi0"; + reg = <0x000000 0x080000>; + }; + + partition@80000 { + label = "wifi1"; + reg = <0x080000 0x080000>; + }; + }; }; };