Re: DT connectors, thoughts

[David Gibson]

On Thu, Jul 21, 2016 at 02:15:57PM -0500, Rob Herring wrote:
> On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
> <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > Hi,
> >
> > Here's some of my thoughts on how a connector format for the DT could
> > be done.  Sorry it's taken longer than I hoped - I've been pretty
> > swamped in my day job.
> >
> > This is pretty early thoughts, but gives an outline of the approach I
> > prefer.
> >
> > So.. start with an example of a board DT including a widget socket,
> > which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.
> >
> > /dts-v1/;
> >
> > / {
> >         compatible = "foo,oldboard";
> >         ranges;
> >         soc@... {
> >                 ranges;
> >                 mmio: mmio-bus@... {
> >                         #address-cells = <2>;
> >                         #size-cells = <2>;
> >                         ranges;
> >                 };
> >                 i2c: i2c@... {
> >                 };
> >                 intc: intc@... {
> >                         #interrupt-cells = <2>;
> >                 };
> >         };
> >
> >         connectors {
> >                 widget1 {
> >                         compatible = "foo,widget-socket";
> >                         w1_irqs: irqs {
> >                                 interrupt-controller;
> >                                 #address-cells = <0>;
> >                                 #interrupt-cells = <1>;
> >                                 interrupt-map-mask = <0xffffffff>;
> >                                 interrupt-map = <
> >                                         0 &intc 7 0
> >                                         1 &intc 8 0
> >                                 >;
> >                         };
> >                         aliases = {
> >                                 i2c = &i2c;
> >                                 intc = &w1_irqs;
> 
> I understand how you are using i2c alias, but not the intc. It would
> help if the same names were not used in multiple places unless they
> are the same thing.

Yes, sorry.  We have both the /soc/intc node which is the base board's
master interrupt controller.  Then we have the connector local 'intc'
alias which describes the local interrupt space for just the
connector.

> What does using aliases here buy us vs. just properties with a
> phandle?

Um.. I'm not sure what you mean.

> >                                 mmio = &mmio;
> >                         };
> >                 };
> >         };
> > };
> >
> > Note that the symbols are local to the connector, and explicitly
> > listed, rather than including all labels in the tree.  This is to
> > enforce (or at the very least encourage) plugins to only access those
> > parts of the base tree.
> >
> > Note also the use of an interrupt nexus node contained within the
> > connector to control which irqs the socketed device can use.  I think
> > this needs some work to properly handle unit addresses, but hope
> > that's enough to give the rough idea.
> >
> > So, what does the thing that goes in the socket look like?  I'm
> > thinking some new dts syntax like this:
> >
> > /dts-v1/;
> >
> > /plugin/ foo,widget-socket {
> >         compatible = "foo,whirligig-widget";
> > };
> >
> > &i2c {
> >         whirligig-controller@... {
> >                 ...
> >                 interrupt-parent = <&widget-irqs>;
> >                 interrupts = <0>;
> >         };
> > };
> >
> > Use of the /plugin/ keyword is rather different from existing
> > practice, so we may want a new one instead.
> >
> > The idea is that this would be compiled to something like:
> >
> > /dts-v1/;
> >
> > / {
> >         socket-type = "foo,widget-socket";
> >         compatible = "foo,whirligig-widget";
> >
> >         fragment@0 {
> >                 target-alias = "i2c";
> 
> Yet another way to express the target... Every new feature for
> overlays seems to define a new way.

Well, yes.  Frankly I think that's because the original ways of
describing the target were not well thought out.  Using a phandle is
awkward because it will always be -1 until fixups are applied, which
seems a bit pointless (why not have the alias/label directly).  Using
a full path means that the overlay can overwrite anywhere in the base
tree which doesn't seem like good practice.

> My thinking was the target is a
> connector node and all devices are under it.

I thought about this, and I think it's technically possible, but it
gets really ugly.  The trouble is that connectors frequently have pins
onto multiple buses.  In order to combine those into a single parent
node, we'd have to combine the address spaces of all those buses.
That can be done using some complex multi-cell encoding, but it won't
be pretty.  Then to make the connection point be a single node in the
base tree you'd essentially have to combine all the buses used in the
connector throughout the base board DT, so that ugly multi-cell
encoding will have to be used throughout most of the base DT, not just
in the connector stuff.

> In your case, the
> connector is not part of the hierarchy for any devices in the overlay.
> That may simplify adding OS support, but seems to be a less accurate
> representation of the h/w.

I think its the best we can do.  Because connectors usually combine
multiple logically distinct buses, they really don't exist as a
well-defined point in a single heirarchy.

> >                 __overlay__ {
> >                         whirligig-controller@... {
> >                                 ...
> >                                 interrupt-parent = <0xffffffff>;
> >                                 interrupts = <0>;
> >                         };
> >                 };
> >         };
> >         __phandle_fixups__ {
> >                 /* These are (path, property, offset) tuples) */
> >                 widget-irqs =
> >                         "/fragment@0/__overlay__/whirligig-controller@...",
> >                         "interrupt-parent", <0>;
> >         };
> > };
> 

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