On Tue, May 19, 2015 at 10:25:58AM +0200, Jonas Gorski wrote: > On Tue, May 19, 2015 at 12:49 AM, Brian Norris > <computersforpeace@xxxxxxxxx> wrote: > > On Sun, May 17, 2015 at 07:35:12PM +0200, Hauke Mehrtens wrote: > >> --- a/drivers/mtd/mtdpart.c > >> +++ b/drivers/mtd/mtdpart.c ... > >> @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p) > >> +static const char * const *of_get_probes(struct device_node *dp) > >> +{ ... > >> + cp = of_get_property(dp, "linux,part-probe", &cplen); > > > > I see this is not property is not documented in the first place. If > > we're going to make it generically useful, we definitely need to > > document it, including what the supported partition probe names are. > > > > I'm also kind of ambivalent on this point, but I might as well ask: does > > the 'linux,...' naming make sense? We're not really supposed to be > > putting pure software information in the device tree, let alone info > > that's specific to Linux. I see how the partition parser could be (and > > should be, really) considered OS-agnostic, though, and might deserve a > > spot in the DT, at least if it's not safe to run arbitrary partition > > parsers on all MTDs. > > As long as we use the direct parser names, we should keep the linux prefix. > > Actually a common issue is that many firmware partition tags aren't > capable of describing the full flash layout, so partition parsers have > to "cheat" and make a lot of assumptions, e.g. the location and the > size of the bootloader. So wo would need to have a mix of static > partitions (bootloader, bootloader config, calibration data) and > dynamic ones (kernel, rootfs, overlay). OK, so this much seems to suggest that we can't really determine a set of parsers that apply to all MTDs, and we have to stick with some kind of platform information for choosing a set of custom parsers. > So ideally we would pass the > static partitions as device tree nodes, and let the partition parsers > then be applied to partitions instead of the full mtd device. This > would also work as a safeguard against mis-guessing important > partitions like calibration data which aren't recoverable in case they > get overwritten. > > So my first Idea would be having something like this: [...] Not commenting on this portion yet. I'll read the others' comments later, but I think that's veering a little off the track of the original problem. > >> @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > >> { > >> struct mtd_part_parser *parser; > >> int ret = 0; > >> + const char *const *types_of = NULL; > >> + > >> + if (data && data->of_node) { > >> + types_of = of_get_probes(data->of_node); > >> + if (types_of != NULL) > >> + types = types_of; > >> + } > > > > I'm not sure I really like the semantics here. This means that the > > device tree can override all Linux defaults. So we can't, for instance, > > have the cmdline parser override all other selections (e.g., for > > debugging), or make sure that 'ofpart' is available for all MTDs. > > Letting the device tree override cmdline parsers is actually a valid > use case for OpenWrt, where the bootloader passes a > different/incompatible partition layout through mtdparts than what is > actually used by the firmware on flash. That seems like a sucky firmware, and not an argument for changing how Linux works. Can't you override their cmdline somehow? > If we want to make it overrideable for debug purposes adding a > commandline parameter like > mtdparser="<mtdname>:<parser1[,parser2,...]>" might be a better > option. > > >> > >> if (!types) > >> types = default_mtd_part_types; > > > > In fact, I don't really like this existing hunk either ^^^. It has the > > same effect, of killing things like cmdlinepart, which should be > > available on all MTDs. > > default_mtd_part_types contains cmdline part, so it should be > available unless they pass their own parsers. Right, but (1) I don't like each driver deciding which parsers are available; platform setup should be independent of the flash/controller driver, and (2) the cmdline partition parser and ofpart parsers should always be available for use on all drivers; to break that is a bug, IMO. I think Hauke brought up a similar point to (1). Brian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html