On Sep 15, 2013, at 9:57 PM, Benjamin Herrenschmidt wrote: > [resent to the right list this time around] > > Hi folks ! > > So I don't have the bandwidth to follow closely what's going on, but I > just today noticed the crackpot that went into 3.11 as part of commit: > > 9d8eab7af79cb4ce2de5de39f82c455b1f796963 > drivers: of: add initialization code for dma reserved memory > > Fist of all, do NOT add (or change) a binding as part of a patch > implementing code, it's gross. > > Secondly, I don't know how much that binding was discussed on the list > (I assume it was and I just missed it) but it's gross. > > It duplicates a binding that Jeremy Kerr had proposed a while ago for > a /reserved-ranges (and /reserved-names) pair of properties, possibly in > a better way but the fact is that the original binding received little > or no feedback and we went on and implemented support for it in powerpc > back in early 3.11 merge window. > > Additionally, it has the following issues: > > - It describes the "memory" node as /memory, which is WRONG > > It should be "/memory@unit-address, this is important because the Linux > kernel of_find_device_by_path() isn't smart enough to do partial > searches (unlike the real OFW one) and thus to ignore the unit address > for search purposes, and you *need* the unit address if you have > multiple memory nodes (which you typically do on NUMA machines). > > - To add to the above mistake, it defines "reserved memory" as a child > node of the "/memory" node. That is wrong or at least poorly thought > out. There can be several "memory" nodes, representing different areas > of memory, possibly even interleaved, having the reserved ranges as > children of a specific memory nodes thus doesn't work very well. > Breaking them up into regions based on what memory nodes they cover is > really nasty. Basically, the "reserved-memory" node should have been at > the root of the device-tree. > > - It provides no indication of what a given region is used for (or used > by). In the example, "display_region" is a label (thus information that > is lost) and unless it's referenced by another node there is no good way > to know what this region is about which is quite annoying. > > - The "memory-region" property is a phandle to a "reserved-memory" > region, this is not intuitive. If anything, the property should have > been called "reserved-memory-region". > > - The way the "compatible" property is documented breaks a basic > premise that the device-tree is a hardware description and not some > convenient way to pass linux specific kernel parameters accross. It is > *ok* to pass some linux specific stuff and to make compromise based on > what a driver generally might expect but the whole business of using > that to describe what to put in CMA is pushing it pretty far ... > > - The implementation of early_init_dt_scan_reserved_mem() will not work > on a setup whose /memory node has a unit address (typically /memory@0) > > Now, I'd like to understand why not use the simpler binding originally > proposed by Jeremy, which had the advantage of proposing a unique name > per region in the traditional form "vendor,name", which then allows > drivers to pick up the region directly if they wish to query, remove or > update it in the tree for example (because they changed the framebuffer > address for example and want kexec to continue working). > > I don't object to having a node per region, though it seemed unnecessary > at the time, but in any case, the current binding is crap and need to be > fixed urgently before its use spreads. > > Ben. Where is Jermey's binding documented ? Is there concern of "breaking" whatever got merged in powerpc? - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- 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
