On Fri, Mar 22, 2019 at 03:49:56AM -0700, Frank Rowand wrote: > 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. I have a hard time imagining how this is supposed to work. If the phandle is generated at runtime, how will anyone know how to reference it in an overlay? Unless they generate the overlay at runtime, in which case why bother with the overlay mechanism at all? > >> 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. I'm not sure what you mean by base DTB, but there are a number of reasons why there may be a device tree node for a display controller but no need for the carveout. For example a DTB may list a node for a display controller, but the display controller may end up not being used if the user didn't connect a monitor. Or a user may want to just disable the display controller (or not compile/load the driver) if they want to run the device in headless mode. Secondly, bootloaders may not implement display support at all, so even if it's possible to output something on a screen and users have connected a screen, the bootloader may just not support it. So the existence of the carveout is dependent on so many things that it's a bit of a stretch to require the DTS file to contain carveouts for every possible scenario. One other thing to consider in this context is that a carveout is not hardware description, so a lot of DTBs exist that don't contain any such information. This means that extending bootloaders with functionality that require the creation of a carveout would effectively be a device tree ABI break. Thirdly the carveout will often be allocated dynamically. While we can fill in any values at runtime, in order to create a "conformant" DTB we would also need to rename the nodes so that they can contain the proper unit-addresses. This all ends up with pretty much the same code as we'd need to generate all of the nodes from scratch. Lastly there's also the issue that I've seen bootloaders fail to deal with carveouts that have zero size. You could argue that that's a bug in the bootloader implementation, but then again, I don't think it's a big stretch for users to assume that if there is no reserved memory there's no need to have a node at all, so not handling the zero-size carveout case is something that I'd think is fairly common. > > 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. Like I said, I don't think this is really an issue that will happen. My understanding is that overlays are built on top of a base DTB that needs to be (or at least usually will be) available at compile time. Users of fdt_generate_phandle() will only ever create the phandles way after the creation of DTBs or overlays, so I don't think this would be an issue. Even if there was an issue, there's nothing preventing us from just rejecting both to work together. You already said that an overlay can't be applied if there's no metadata for a given phandle. So we already have these checks in place and people will just not be able to do this. You're arguing that we should not be adding support for generating phandle values at runtime because it may break in one specific use-case. That's a bit like saying people shouldn't be allowed to wear shoes because they may trip after stepping on their own shoelaces. Thierry
Attachment:
signature.asc
Description: PGP signature
