Re: [PATCH v7 3/5] dtc: Document the dynamic plugin internals

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5711 bytes --]

On Wed, Jun 29, 2016 at 07:59:13PM -0700, Frank Rowand wrote:
> Hi Pantelis,
> 
> On 05/29/16 21:22, David Gibson wrote:
> 
> < massive snip >
> 
> >>> What other properties are you envisioning?  (Looking for the architectural
> >>> vision that you have.)
> >>>
> >>
> >> Oh, there are a lot of properties that can be provided.
> >>
> >> For instance you can declare manufacturing info (like part numbers, version numbers,
> >> serial numbers that can be used for quirking). You can declare things like load order
> >> when you need precedence of overlays (i.e. on the bone the soldered on hdmi output
> >> should be disabled when an add on cape with display capability is attached).
> >> You can declare resources (i.e. pins or power draw figures) to make a decision
> >> whether enabling an expansion board is safe.
> >>
> >> I’m sure more ideas will come when we put it into wide-spread use.  
> > 
> > Yeah.  I'm not entirely sure I'm convinced by the specific examples
> > given so far.  However, in general I can see the value in providing a
> > way we can extend to add more metadata.  The two level structure with
> > __overlay__ gives us that, whereas the one level approach doesn't.
> 
> I'm not convinced about putting additional properties (beyond "target") in the
> fragment nodes.  And I still find the two extra levels of "fragment" nodes and
> "__overlay__" nodes extra complexity, and that the complexity is especially
> unwelcome if the overlay dts is hand written (as the beagle cape overlays
> that I have seen appear to be).  HOWEVER, I learned something new today that
> makes me more comfortable with the two extra node levels.  Here is an example:
> 
> $ cat ex_1_overlay.dts 
> 
> /dts-v1/;
> 
> /plugin/;
> 
> &tree_1 {
> 	remote_prop = <0xfeedfeed &foo>;
> };
> 
> $ dtc -@ -O dts ex_1_overlay.dts 
> Warning (unit_address_vs_reg): Node /fragment@0 has a unit name, but no reg property
> /dts-v1/;
> 
> / {
> 
> 	fragment@0 {
> 		target = <0xffffffff>;
> 
> 		__overlay__ {
> 			remote_prop = <0xfeedfeed 0xffffffff>;
> 		};
> 	};
> 
> 	__symbols__ {
> 	};
> 
> 	__fixups__ {
> 		tree_1 = "/fragment@0:target:0";
> 		foo = "/fragment@0/__overlay__:remote_prop:4";
> 	};
> 
> 	__local_fixups__ {
> 	};
> };
> 
> That example is using dtc from:
>   url = https://github.com/pantoniou/dtc
>   branch: dt-overlays8
>   as of commit: 6f4db2fc2354
> 
> I do not know if that is current or not.
> 
> So the nice thing about that is that the overlay source file does not have to
> provide the fragment and __overlay__ nodes.  They just magically appear in the
> compiled blob.  Is that behavior that I can count on continuing to exist in dtc?

Yes.  There are still some unclear points about how we want to
integrate this into upstream dtc.  However, automatically translating
the existing dts overlay syntax into the dtb encoding, including
constructing the fragment@ and other special nodes is absolutely the
intention.

> Note the warning about no reg property in /fragment@0.

Yes - pretty much the next thing on my list when I get another chance
to look at this is how to properly apply checks in the preence of
overlays.  My intention is that some will be flagged as being run on
each overlay fragment individually, others only on the fully
constructed tree (and so not at all when in plugin mode).

This should address a number of problems with checks and overlays,
including this one.

> The other issue with the device tree source in my example is that I don't think
> there is a way to add extra properties to node "fragment@0" in the general case
> where there are multiple fragments.  It _is_ possible to add a property as I
> show in the next example, but it seems fragile to be able to count on the order
> and names of fragments auto generated by dtc.  (And again, I don't really want
> those extra properties anyway.)

Right, the below is indeed fragile.  I expect it will break once we've
fully implemented the upstream approach to overlays I'm intending -
that will delay resolution of the overlays until after the input is
fully parsed.

My expectation was that any extra properties in the fragment@ nodes
would be metadata and would therefore either be auto-generated by dtc,
or we'd create special dtc syntax to populate it if necessary.

> Example of adding a property to fragment@0:
> 
> $ cat ex_1b_overlay.dts
> 
> /dts-v1/;
> 
> /plugin/;
> 
> &tree_1 {
> 	remote_prop = <0xfeedfeed &foo>;
> };
> 
> / {
> 	fragment@0 {
> 		another_fragment_prop = "frag property";
> 	};
> };
> 
> $ dtc -@ -O dts ex_1b_overlay.dts 
> Warning (unit_address_vs_reg): Node /fragment@0 has a unit name, but no reg property
> /dts-v1/;
> 
> / {
> 
> 	fragment@0 {
> 		target = <0xffffffff>;
> 		another_fragment_prop = "frag property";
> 
> 		__overlay__ {
> 			remote_prop = <0xfeedfeed 0xffffffff>;
> 		};
> 	};
> 
> 	__symbols__ {
> 	};
> 
> 	__fixups__ {
> 		tree_1 = "/fragment@0:target:0";
> 		foo = "/fragment@0/__overlay__:remote_prop:4";
> 	};
> 
> 	__local_fixups__ {
> 	};
> };
> 
> 
> >>> If load manager specific details are appropriate in the devicetree (a whole
> >>> different discussion) then maybe a /chosen/load-manager node could exist to
> >>> hold them instead of putting them in /, where the patch currently locates
> >>> "/* various properties for loader use; i.e. part id etc. */".
> 
> -Frank
> 

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]