On 3/21/19 3:26 AM, Thierry Reding wrote: > On Wed, Mar 20, 2019 at 05:36:15PM -0700, Frank Rowand wrote: >> On 3/20/19 8:10 AM, Thierry Reding wrote: >>> From: Thierry Reding <treding@xxxxxxxxxx> >>> >>> The new fdt_generate_phandle() function can be used to generate a new, >>> unused phandle given a specific device tree blob. The implementation is >>> somewhat naive in that it simply walks the entire device tree to find >>> the highest phandle value and then returns a phandle value one higher >>> than that. A more clever implementation might try to find holes in the >>> current set of phandle values and fill them. But this implementation is >>> relatively simple and works reliably. >>> >>> Also add a test that validates that phandles generated by this new API >>> are indeed unique. >> >> I don't see any issues. >> >> I am curious as to what the envisioned use is for fdt_generate_phandle(). > > The idea here is to allow firmware to pass information about carveout > areas to the Linux kernel (and potentially other OSes). To specify the > carveouts, it needs to create subnodes in the top-level /reserved-memory > node and attach a phandle to those nodes so that they can later be > referenced by users of the carveouts. > > One such example would be a framebuffer carveout set up by early > firmware to show a boot splash. In order to prevent the OS kernel from > recycling the framebuffer carveout for its allocator, the firmware needs > to marks it as reserved memory. This is what the child nodes in the top- > level /reserved-memory node do. Then the display controller that's > scanning out the framebuffer from that carveout needs to be passed a > reference to the reserved memory node (via its phandle, which is stored > in the display controller node's memory-region property). > > The display controller driver can then use that memory-region property > to look up the reserved memory region and reuse it, copy out data, ... > This information can also be used on early OS boot to setup things like > IOMMU page tables so that there's initially a 1:1 mapping of physical > to IO virtual addresses for the carveout. This is necessary because the > display controller may otherwise cause a lot of IOMMU faults trying to > access a physical memory address through the IOMMU. > > Does that clarify the use-case? Yes, thanks. That is an excellent description. I think that it is not good to provide a mechanism to create phandles in libfdt, because people are likely to use the mechanism if it exists. How is that for a rationale? :-) The problem is that such a phandle is entirely outside the "architecture" (and I use that word loosely) of phandle related infrastructure used for overlays. And I am not just thinking of current implementation, but also thinking of how I am hoping the implementation will change. Either the phandle overlay related metadata will not be available, or fdt_generate_phandle() would have to add the complication of also creating that metadata (which I also think is not a good idea). Is there an alternative approach to achieve the same required use-case? One possibility that jumped to mind (but I haven't thought through yet) is to put the possible carveout area definitions in the base dts source, but with a size of zero, where size zero means not active. Then the firmware could update the size (and offset if needed) to create the actual carveout. -Frank > Thanks, > Thierry > >>> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> >>> --- >>> Changes in v3: >>> - add fdt_generate_phandle() to version.lds >>> >>> Changes in v2: >>> - Handle the case where the number of phandles has been exhausted. Also >>> add more testing to exercise this corner case. Note that this is tied >>> to the current, naive implementation of fdt_generate_phandle(). If it >>> is ever changed to be more clevel, the tests will have to be updated. >>> --- >>> libfdt/fdt_ro.c | 31 +++++++++++++++++++++++++++++++ >>> libfdt/libfdt.h | 19 +++++++++++++++++++ >>> libfdt/libfdt_env.h | 1 + >>> libfdt/version.lds | 1 + >>> tests/get_phandle.c | 31 ++++++++++++++++++++++++++++++- >>> tests/multilabel.dts | 5 +++++ >>> tests/multilabel_merge.dts | 5 +++++ >>> tests/references.c | 19 +++++++++++++++++-- >>> tests/references.dts | 5 +++++ >>> 9 files changed, 114 insertions(+), 3 deletions(-) < snip >
