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. > 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. 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. 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 >
Attachment:
signature.asc
Description: PGP signature
