On 4/3/19 1:05 PM, Frank Rowand wrote:
> On 3/25/19 1:18 AM, Thierry Reding wrote:
>> 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?
>
> Overlays reference phandles by name.
>
> If a phandle value is generated at runtime, then the value will not be
> available to overlays unless the phandle name metadata is also added
> to the devicetree.
>
> Which is exactly the point I originally made.
>
>
>>>>> 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.
>
> Yes, but no more than existing cases of disabled nodes. Note that I agree
> with your statement, just not sure how strong an argument it is.
>
>
>>> 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
>
> 'base DTB' is the devicetree that is provided to the Linux kernel when
> it it booted. (This kernel centric view treats any overlay that the
> bootload applies as part of the 'base DTB'.)
>
>
>> 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.
>
> The driver will still be loaded unless the display controller node status
> is disabled.
>
>
>> 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.
>
> Yes. If the node is disabled then the driver should not be loaded. If
> the driver is not compiled then the driver will not be loaded.
>
> But in both of those cases the bootloader does not know the carve out is
> not needed. So I don't understand the point.
>
>
>> 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.
>
> I did not propose requiring the creation of a carveout. I proposed a
> possible alternate way of defining the default (empty) carveout in the DTS.
>
> The ABI change (not break) would be that the new DTS (with an empty carveout)
> would require a newer driver version that is able to detect that the
> carveout referenced my the optional memory-region property is empty, and
> thus ignore it. Exitsing DTBs would continue to work with the new
> driver version, thus not an 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.
>
>>From the Linux kernel Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt,
>
> Following the generic-names recommended practice, node names should
> reflect the purpose of the node (ie. "framebuffer" or "dma-pool"). Unit
> address (@<address>) should be appended to the name if the node is a
> static allocation.
>
> If I read that correctly, unit-address is optional in this case because
> the node is not a static allocation.
>
>
>> 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.
>
> Agreed, as I said above this would require updating the display driver
> bindings and driver sources.
>
>
>>>> 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.
>
> Incorrect. The base DTB is not involved in compiling the overlay DTS. It
> is even possible to apply the same overlay to different base DTBs.
>
> At some point in the future, there will be a desire to be able to validate
> the overlay DTS against the bindings, and in my mind that requires the
> base DTB (or DTS) _and_ the same for any other overlay applied first. But
> this issue is theoretical at this point.
>
>> 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.
>
> Yes. We can make overlays that contain display device notes second class
^^^^^
nodes
> citizens in this case.
>
> If that is the case then it should be clearly and visibly documented as
> a limitation / constraint / or pick your preferred word.
>
>
>> 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.
>
> No. I am arguing that if phandles are generated at bootloader runtime
> then we should consider whether there is a reasonable way to make those
> phandles available to overlays, just like all phandles can be made
> available to overlays.
>
>
>> 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
>>
>
>
