| Message ID | 20170424124120.31613-4-zajec5@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi, On 24 April 2017 at 14:41, Rafał Miłecki <zajec5@gmail.com> wrote: > From: Brian Norris <computersforpeace@gmail.com> > > Partition parsers can now provide an of_match_table to enable > flash<-->parser matching via device tree. > > This support is currently limited to built-in parsers as it uses > request_module() and friends. This should be sufficient for most cases > though as compiling parsers as modules isn't a common choice. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > Acked-by: Brian Norris <computersforpeac@gmail.com> > --- > This is based on Brian's patches: > [RFC PATCH 4/7] mtd: add of_match_mtd_parser() and of_mtd_match_mtd_parser() helpers > [RFC PATCH 6/7] RFC: mtd: partitions: enable of_match_table matching > > V1: Put helpers in mtdpart.c instead of drivers/of/of_mtd.c > Merge helpers into a single of_mtd_match_mtd_parser > --- > drivers/mtd/mtdpart.c | 47 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/mtd/partitions.h | 1 + > 2 files changed, 48 insertions(+) > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index 73c52f1a2e4c..d0cb1a892ed2 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -861,6 +861,41 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser, > return ret; > } > > +static bool of_mtd_match_mtd_parser(struct mtd_info *mtd, > + struct mtd_part_parser *parser) > +{ > + struct device_node *np; > + bool ret; > + > + np = mtd_get_of_node(mtd); > + np = of_get_child_by_name(np, "partitions"); > + > + ret = !!of_match_node(parser->of_match_table, np); > + > + of_node_put(np); > + > + return ret; > +} > + > +static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd) > +{ > + struct mtd_part_parser *p, *ret = NULL; > + > + spin_lock(&part_parser_lock); > + > + list_for_each_entry(p, &part_parsers, list) { > + if (of_mtd_match_mtd_parser(mtd, p) && > + try_module_get(p->owner)) { > + ret = p; > + break; > + } > + } Hm, maybe iterate over the compatibles, so parsers matching the most specific compatible get precedence in case there is more than one compatible? Currently it will match the first one that matches any compatible, and registration order of parsers can change that. I'm thinking of parsers that partially rely on fixed, unprobable layouts, so can use "fixed-partitions" as a fallback compatible. E.g. having something like this partitions { compatible = "sample,custom-layout", "fixed-partitions"; bootloader@0 { ... }; firmware@10000 { .... }; /* will be split by the parser */ extra@780000 { .... }; /* partition the on-flash format can't specify */ }; Where you will still be able to write an image raw to the image partition even if the "custom-layout"-parser isn't present/enabled, but if it is present, it should always be used. Regards Jonas
On 04/24/2017 05:31 PM, Jonas Gorski wrote: > On 24 April 2017 at 14:41, Rafał Miłecki <zajec5@gmail.com> wrote: >> From: Brian Norris <computersforpeace@gmail.com> >> >> Partition parsers can now provide an of_match_table to enable >> flash<-->parser matching via device tree. >> >> This support is currently limited to built-in parsers as it uses >> request_module() and friends. This should be sufficient for most cases >> though as compiling parsers as modules isn't a common choice. >> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> Acked-by: Brian Norris <computersforpeac@gmail.com> >> --- >> This is based on Brian's patches: >> [RFC PATCH 4/7] mtd: add of_match_mtd_parser() and of_mtd_match_mtd_parser() helpers >> [RFC PATCH 6/7] RFC: mtd: partitions: enable of_match_table matching >> >> V1: Put helpers in mtdpart.c instead of drivers/of/of_mtd.c >> Merge helpers into a single of_mtd_match_mtd_parser >> --- >> drivers/mtd/mtdpart.c | 47 ++++++++++++++++++++++++++++++++++++++++++ >> include/linux/mtd/partitions.h | 1 + >> 2 files changed, 48 insertions(+) >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> index 73c52f1a2e4c..d0cb1a892ed2 100644 >> --- a/drivers/mtd/mtdpart.c >> +++ b/drivers/mtd/mtdpart.c >> @@ -861,6 +861,41 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser, >> return ret; >> } >> >> +static bool of_mtd_match_mtd_parser(struct mtd_info *mtd, >> + struct mtd_part_parser *parser) >> +{ >> + struct device_node *np; >> + bool ret; >> + >> + np = mtd_get_of_node(mtd); >> + np = of_get_child_by_name(np, "partitions"); >> + >> + ret = !!of_match_node(parser->of_match_table, np); >> + >> + of_node_put(np); >> + >> + return ret; >> +} >> + >> +static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd) >> +{ >> + struct mtd_part_parser *p, *ret = NULL; >> + >> + spin_lock(&part_parser_lock); >> + >> + list_for_each_entry(p, &part_parsers, list) { >> + if (of_mtd_match_mtd_parser(mtd, p) && >> + try_module_get(p->owner)) { >> + ret = p; >> + break; >> + } >> + } > > > Hm, maybe iterate over the compatibles, so parsers matching the most > specific compatible get precedence in case there is more than one > compatible? Currently it will match the first one that matches any > compatible, and registration order of parsers can change that. I'm > thinking of parsers that partially rely on fixed, unprobable layouts, > so can use "fixed-partitions" as a fallback compatible. > > E.g. having something like this > > partitions { > compatible = "sample,custom-layout", "fixed-partitions"; > > bootloader@0 { ... }; > > firmware@10000 { .... }; /* will be split by the parser */ > > extra@780000 { .... }; /* partition the on-flash format can't specify */ > }; > > Where you will still be able to write an image raw to the image > partition even if the "custom-layout"-parser isn't present/enabled, > but if it is present, it should always be used. I see the point, but I'm afraid we're lacking some DT helper for this. See below for the function I wrote (and I'm not proud of) - compile tested only. I think we would need a new helper similar to the of_match_node: 1) Taking const struct of_device_id *matches 2) Taking const struct device_node *node but returning a score of the best match. DT guys: any comment on this? Rob? Would this be acceptable to: 1) Take this patch as is as Linux current doesn't support other bindings 2) Work on DT helper + mtd modification in a separated patchset? static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd) { struct mtd_part_parser *p, *ret = NULL; struct device_node *np; struct property *prop; const char *cp; np = mtd_get_of_node(mtd); np = of_get_child_by_name(np, "partitions"); if (!np) return NULL; spin_lock(&part_parser_lock); of_property_for_each_string(np, "compatible", prop, cp) { list_for_each_entry(p, &part_parsers, list) { const struct of_device_id *matches; for (matches = p->of_match_table; matches->name[0] || matches->type[0] || matches->compatible[0]; matches++) { if (!of_compat_cmp(cp, matches->compatible, strlen(matches->compatible)) && try_module_get(p->owner)) { ret = p; break; } } if (ret) break; } if (ret) break; } spin_unlock(&part_parser_lock); of_node_put(np); return ret; }
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 73c52f1a2e4c..d0cb1a892ed2 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -861,6 +861,41 @@ static int mtd_part_do_parse(struct mtd_part_parser *parser, return ret; } +static bool of_mtd_match_mtd_parser(struct mtd_info *mtd, + struct mtd_part_parser *parser) +{ + struct device_node *np; + bool ret; + + np = mtd_get_of_node(mtd); + np = of_get_child_by_name(np, "partitions"); + + ret = !!of_match_node(parser->of_match_table, np); + + of_node_put(np); + + return ret; +} + +static struct mtd_part_parser *mtd_part_get_parser_by_of(struct mtd_info *mtd) +{ + struct mtd_part_parser *p, *ret = NULL; + + spin_lock(&part_parser_lock); + + list_for_each_entry(p, &part_parsers, list) { + if (of_mtd_match_mtd_parser(mtd, p) && + try_module_get(p->owner)) { + ret = p; + break; + } + } + + spin_unlock(&part_parser_lock); + + return ret; +} + /** * parse_mtd_partitions - parse MTD partitions * @master: the master partition (describes whole MTD device) @@ -913,6 +948,18 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, if (ret < 0 && !err) err = ret; } + + parser = mtd_part_get_parser_by_of(master); + if (!parser) + return err; + + ret = mtd_part_do_parse(parser, master, pparts, data); + if (ret > 0) + return 0; + mtd_part_parser_put(parser); + if (ret < 0 && !err) + err = ret; + return err; } diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h index 2787e76c030f..073e1d8d5d17 100644 --- a/include/linux/mtd/partitions.h +++ b/include/linux/mtd/partitions.h @@ -77,6 +77,7 @@ struct mtd_part_parser { struct list_head list; struct module *owner; const char *name; + const struct of_device_id *of_match_table; int (*parse_fn)(struct mtd_info *, const struct mtd_partition **, struct mtd_part_parser_data *); void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);