On Thu, Jan 04, 2018 at 09:02:38PM -0800, Frank Rowand wrote: > On 01/04/18 19:50, Kyle Evans wrote: > > On Thu, Jan 4, 2018 at 9:14 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote: > >> On 01/04/18 18:39, Kyle Evans wrote: > >>> On Thu, Jan 4, 2018 at 7:55 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote: > >>>> On 01/04/18 13:47, Kyle Evans wrote: > >>>>> On Thu, Jan 4, 2018 at 3:34 PM, Kyle Evans <kevans@xxxxxxxxxxx> wrote: > >>>>>> On Thu, Jan 4, 2018 at 2:41 PM, Kyle Evans <kevans@xxxxxxxxxxx> wrote: > >>>>>>> On Thu, Jan 4, 2018 at 2:33 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote: > >>>>>>>> [... snip ...] > >>>>>>>> > >>>>>>>> Does this remove the need for the proposed patch, or am I still > >>>>>>>> missing something? > >>>>>>> > >>>>>>> ... nope. Apparently I never tested this with this particular dtc(1) > >>>>>>> and instead just assumed it did the same as ours- allocate phandle > >>>>>>> sparsely, even with -@. That certainly removes the need for this > >>>>>>> patch, and I'm somewhat upset that I hadn't previously considered > >>>>>>> this. > >>>>>>> > >>>>>>> @David, Jon: Please disregard all of the patches along these lines... > >>>>>>> I'll fix this in our dtc, where it should be fixed. > >>>>>>> > >>>>>>> Thanks, Frank! > >>>>>> > >>>>>> Actually, I'm kind of torn on whether this is useful or not. With > >>>>>> being able to have EFI-provided FDT, it's hard to guarantee whether > >>>>>> the FDT we're provided has been compiled with GPL dtc(1) and -@. The > >>>>>> above solves this problem for most of my personal use-cases , though, > >>>>>> since I can guarantee that our FDT and U-Boot provided FDT is compiled > >>>>>> properly. > >>>>> > >>>>> Apologies for the triple post; I realized that this argument is > >>>>> inherently wrong, since we can't reference the node if there's no > >>>>> symbol anyways. > >>>>> > >>>>> The only way this might still be a good idea is to support more > >>>>> minimal cases where an implementation might prefer to not create a > >>>>> phandle for nodes that haven't been referenced. > >>>>> > >>>>> In our case, we have a function [1] that walks the tree and generates > >>>>> metadata on nodes that have phandles, under the assumption that these > >>>>> have been referenced somewhere and provides a way to more quickly > >>>>> reference these specifically through a separate linked link. > >>>>> Allocating phandles for everything as GPL dtc does adds quite a bit > >>>>> more overhead to this. > >>>>> > >>>>> [1] http://src.illumos.org/source/xref/freebsd-head/sys/dev/ofw/openfirm.c#119 > >>>> > >>>> The "-@" option does not add a phandle to _every_ node, just to nodes > >>>> that have a label: > >>>> > >>> > >>> In practice, this is still a not necessarily close superset of those > >>> that are actually going to actually get referenced in a given .dts. I > >>> note that a number of nodes will still get labelled that are unlikely > >>> to be referenced for any number of reasons. > >>> > >>> For instance, as an example [1][2][3] of one of the boards I'm working > >>> on currently, all of the ehci/ohci/mmc nodes, some set of the pio > >>> nodes... almost every single node has a label, and most of them don't > >>> need a phandle based on what is currently referenced and actually > >>> used. I note that 27 of these nodes seem to be referenced, out of 39 > >>> nodes with labels (numbers are approximate), leaving ~30% (12) of > >>> labelled nodes unreferenced. > >>> > >>> For a slightly larger example, [4][5][6][7]; 74 referenced nodes out > >>> of 113 labelled nodes, leaving ~35% (39) of labelled nodes > >>> unreferenced. > >>> > >>> This is only bothersome because it starts adding up as these things > >>> get bigger, and I don't think it really needs to. The alternative to > >>> keep phandles down to the minimum set required doesn't add much in > >>> maintenance cost or overhead from the overlay application side; > >>> especially for blobs generated by this dtc(1) that make the active > >>> decision to allocate liberally rather than conservatively. > >> > >> This concept relies on a hand coded __symbols__ node instead of > >> having dtc generate it. This makes the overlay devicetree source > >> more fragile. The example dts file in patch 2/2 is fairly safe, > >> but it is legal syntax to specify: > >> > >> test = "/test-node"; > >> > >> instead of: > >> > >> test = &test; > >> > >> And the fragile syntax is exactly what results from a decompiled > >> overlay dtb. > > > > I think I might be misunderstanding something here- our dtc (BSDL > > dtc), with -@, still writes a full /__symbols__ node of all labeled > > nodes, given that this is what -@ is expected to do. The difference is > > I was talking about the .dts file in patch 2/2, where the __symbols__ > node was hand coded, instead of being created by dtc. > > > > that our dtc allocates phandles conservatively because they're a > > concept for cross-referencing nodes. Conceptually, it doesn't seem > > like either way is more or less wrong than the other. > > Yes, the phandles concept is for cross-referencing. In your specific > context, creating phandle properties at the time that applying the > overlay determines that the cross-reference actually occurs does > allow dynamically creating the phandle properties. If overlays > were only applied to flattened device trees, then that could be > a useful optimization. I don't actually see why phandles couldn't be added dynamically for a live tree. In fact adding phandles for nodes that don't have them at unflattening time seems like it might be a good idea on general principle. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
