On 3/22/19 1:58 AM, Thierry Reding wrote: > On Thu, Mar 21, 2019 at 11:37:20AM -0700, Frank Rowand wrote: >> 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). > > I'm not sure I understand how that's relevant to this discussion. DT > overlays and phandles are completely orthogonal. phandles need to work > without knowing anything about overlays. If an overlay is going to be applied on top of a base FDT, and the overlay references a phandle in the base FDT, then the base FDT has to have been compiled with the '-@' option so that extra metadata describing the phandles will be included in the base FDT. Without the extra metadata, the overlay can not be applied. If an overlay needs to reference the phandle created by fdt_generate_phandle() then the overlay apply will fail because the metadata is not in the base FDT. >> 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. > > That's not very practical. First of all, we can't assume that the DTB > can be changed on every device that needs to add carveouts. Also, it is > not necessarily known at the time that the DTS is compiled that there > will even be carveouts. Having them occupy space in the device tree if > they are never used is wasteful and easily confusing. If the base DTB contains a node for a display driver that is capable of detecting a framebuffer carveout, then it is not unlikely that the carveout will exist. It is not especially confusing to have an unused carveout if there is an unambiguous indication of an inactive carveout (such as size zero). We already handle inactive nodes that are marked as such by the status property. > > Also, I don't understand how that would change anything with regards to > your point above about overlay metadata. Surely just because the DTS is > compiled doesn't guarantee that overlay metadata will be produced. Correct, the metadata is only in the DTB if the proper compile flag is specified. But if the carveout (with phandle) is in the DTS and the compile flag is specified, then dtc will create the metadata for that phandle. > > 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 >
