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: 9314 bytes --]

On Fri, May 27, 2016 at 05:52:41PM +0300, Pantelis Antoniou wrote:
> Hi Frank,
> 
> > On May 27, 2016, at 00:31 , Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > On 5/26/2016 10:09 AM, Pantelis Antoniou wrote:
> >> Hi Frank,
> >> 
> >>> On May 26, 2016, at 19:55 , Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>> 
> >>> Hi Pantelis,
> >>> 
> >>> On 5/26/2016 6:49 AM, Rob Herring wrote:
> >>>> On Thu, May 26, 2016 at 2:16 AM, Pantelis Antoniou
> >>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >>>>> Hi David,
> >>>>> 
> >>>>>> On May 26, 2016, at 10:12 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6ryscVyMRj84@public.gmane.org.au> wrote:
> >>>>>> 
> >>>>>> On Thu, May 26, 2016 at 09:36:02AM +0300, Pantelis Antoniou wrote:
> >>>>>>> Hi David,
> >>>>>>> 
> >>>>>>>> On May 26, 2016, at 09:33 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6g@public.gmane.orgid.au> wrote:
> >>>>>>>> 
> >>>>>>>> On Thu, May 26, 2016 at 09:31:20AM +0300, Pantelis Antoniou wrote:
> >>>>>>>>> Hi David,
> >>>>>>>>> 
> >>>>>>>>>> On May 26, 2016, at 09:28 , David Gibson <david-xT8FGy+AXnSZP5U4TJ/6Hg@public.gmane.orgr.id.au> wrote:
> >>>>>>>>>> 
> >>>>>>>>>> On Thu, May 26, 2016 at 09:14:49AM +0300, Pantelis Antoniou wrote:
> >>>>>>>>>>> Hi Frank,
> >>>>>>>>>>> 
> >>>>>>>>>>>> On May 25, 2016, at 22:13 , Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>>>>>>>>> 
> >>>>>>>>>>>> On 5/24/2016 10:50 AM, Pantelis Antoniou wrote:
> >>>>>>>>>>>>> Provides the document explaining the internal mechanics of
> >>>>>>>>>>>>> plugins and options.
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>> Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
> >>> 
> >>> < snip >
> >>> 
> >>>>>>>>>>>>> +So the bar peripheral's DTS format would be of the form:
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +/dts-v1/ /plugin/;    /* allow undefined references and record them */
> >>>>>>>>>>>>> +/ {
> >>>>>>>>>>>>> +      ....    /* various properties for loader use; i.e. part id etc. */
> >>>>>>>>>>>>> +      fragment@0 {
> >>>>>>>>>>>>> +              target = <&ocp>;
> >>>>>>>>>>>>> +              __overlay__ {
> >>>>>>>>>>>>> +                      /* bar peripheral */
> >>>>>>>>>>>>> +                      bar {
> >>>>>>>>>>>>> +                              compatible = "corp,bar";
> >>>>>>>>>>>>> +                              ... /* various properties and child nodes */
> >>>>>>>>>>>>> +                      }
> >>>>>>>>>>>> 
> >>>>>>>>>>>>                   };
> >>>>>>>>>>>> 
> >>>>>>>>>>>>> +              };
> >>>>>>>>>>>>> +      };
> >>>>>>>>>>>>> +};
> >>>>>>>>>>>> 
> >>>>>>>>>>>> Other than the fact that the above syntax is already in the Linux
> >>>>>>>>>>>> kernel overlay implementation, is there a need for the target
> >>>>>>>>>>>> property and the __overlay__ node?  I haven't figured out what
> >>>>>>>>>>>> extra value they provide.
> >>>>>>>>>>>> 
> >>>>>>>>>>>> Without those added, the overlay dts becomes simpler (though for a
> >>>>>>>>>>>> multi-node target path example this would be more complex unless a label
> >>>>>>>>>>>> was used for the target node):
> >>>>>>>>>>>> 
> >>>>>>>>>>>> +/dts-v1/ /plugin/;     /* allow undefined references and record them */
> >>>>>>>>>>>> +/ {
> >>>>>>>>>>>> +       ....    /* various properties for loader use; i.e. part id etc. */
> >>>>>>>>>>>> +       ocp {
> >>>>>>>>>>>> +                       /* bar peripheral */
> >>>>>>>>>>>> +                       bar {
> >>>>>>>>>>>> +                               compatible = "corp,bar";
> >>>>>>>>>>>> +                               ... /* various properties and child nodes */
> >>>>>>>>>>>> +                       };
> >>>>>>>>>>>> +       };
> >>>>>>>>>>>> +};
> >>>>>>>>>>>> 
> >>>>>>>>>>> 
> >>>>>>>>>>> No.
> >>>>>>>>>>> 
> >>>>>>>>>>> That only works if the overlay is applied in a single platform.
> >>>>>>>>>>> 
> >>>>>>>>>>> I have working cases where the same overlay is applied on a ppc and a x86
> >>>>>>>>>>> platform.
> >>>>>>>>>> 
> >>>>>>>>>> Huh?  How so..
> >>>>>>>>>> 
> >>>>>>>>> 
> >>>>>>>>> Yes, it does work. Yes it’s being used right now. It is a very valid use case.
> >>>>>>>>> 
> >>>>>>>>> Think carrier boards on enterprise routers, plugging to a main board
> >>>>>>>>> that’s either ppc or x86 (or anything else for that matter).
> >>>>>>>> 
> >>>>>>>> Sorry, I wasn't clear.  I have no problem believing overlays can be
> >>>>>>>> applied on multiple platforms.
> >>>>>>>> 
> >>>>>>>> What I can't see is how Frank's format breaks that.  AFAICT it
> >>>>>>>> contains exactly the same information in a simpler encoding.
> >>>>>>>> 
> >>>>>>> 
> >>>>>>> It breaks it because it’s missing the target property.
> >>>>>>> 
> >>>>>>> The layout of the base tree is not going to be the same in different
> >>>>>>> platforms, so in the above example ‘ocp’ would not exist in x86 for
> >>>>>>> instance.
> >>>>>> 
> >>>>>> I think you're misinterpreting Frank's suggestion.  As I understand it
> >>>>>> the node names of the top level nodes in his format aren't treated as
> >>>>>> literal node names, but instead treated as label names which are
> >>>>>> resolved similarly to the phandle external fixups.
> >>>>>> 
> >>>>>> Actually.. that is one serious problem with Frank's format, it doesn't
> >>>>>> (easily) allow multiple fragments to be applied to the same target.
> >>>>>> 
> >>>>> 
> >>>>> Ugh, yeah I misinterpreted that. Still, it is not going to work with the patches
> >>>>> I queued with multiple target support.
> >>> 
> >>> OK, so you are talking about the "[RFC] of: Portable Device Tree connector"
> >>> email from 4/27 (just to provide an easy link for everyone).  I'm still
> >>> trying to figure that out.
> >>> 
> >>> So other than that, am I missing something else about what extra
> >>> functionality the extra layers of nodes provides?
> >>> 
> >> 
> >> No, I’m talking about the new target options patchset.
> >> 
> >> "of: overlays: New target methods” & in particular
> >> 
> >> "of: overlay: Implement target index support"
> > 
> > Thanks for the pointer.  I don't think the target options approach
> > is the way to handle the issue (see my reply a couple of minutes
> > ago in that thread).
> > 
> >> 
> >>> 
> >>>> Queued implies accepted which they are not. The multiple ways of
> >>>> expressing targets bothers me. Upstream still has no external
> >>>> interface to overlays, so I think there is still room to change things
> >>>> if we decide it is worthwhile. Better now than stuck with something
> >>>> forever.
> >>>> 
> >>>> I too was wondering about the current syntax before this thread
> >>>> started. We have 2 levels of nodes before we get to any useful
> >>>> information with the current syntax.
> >>>> 
> >> 
> >> That’s on purpose. The first level is to contain load manager specific details,
> >> i.e. part-numbers and other platform specific properties.
> >> 
> >> The second level contains the per fragment properties (at the moment targets).
> > 
> > My suggestion removed the target property.
> 
> Removing the target property requires using a loader. That may be fine
> in general and in fact that’s what the target root methods do (setting
> the target root to ‘/‘ makes them equivalent to your version).

I can't quite make sense of that comment.  No matter the encoding,
you're always going to need some sort of a loader.

> > 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.

> > 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. */".
> 
> Oh yes. But that’s something for us to figure out.
> 
> Regards
> 
> — Pantelis
> 

-- 
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 --]