Re: DT connectors, thoughts

[David Gibson <david@gibson.dropbear.id.au>]

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

On Mon, Aug 29, 2016 at 07:07:49PM -0700, Stephen Boyd wrote:
> Quoting David Gibson (2016-08-29 06:45:11)
> > On Thu, Aug 25, 2016 at 06:38:41PM -0700, Stephen Boyd wrote:
> > > Quoting David Gibson (2016-07-21 21:25:56)
> > > > 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@gibson.dropbear.id.au> wrote:
> > > > > 
> > > > > 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.
> > > 
> > > I think Rob means drop the aliases node and just have:
> > 
> > Oh, ok.  The reason for the aliases node is that putting the aliases
> > (or whatever you want to call them) in the top level connector node
> > limits what potential extensions we can make to the connector format.
> > The aliases can essentially have any property name, so they could
> > collide with additional "metadata" properties we might want to add.
> 
> Agreed. Putting them into a subnode will prevent any collisions, but
> what sorts of collisions would there even be? Presumably the one making
> up the connector binding will be choosing the phandles they want to
> export with specific properties, and during that time they can also
> choose to have other properties that don't conflict?

Hmm.. I suppose.  It still seems conceptually cleaner to me to have
them in their own namespace.

> > > How would we support an expansion board that goes onto two
> > > sockets/connectors provided by the baseboard when the connectors
> > > "export" the same phandle aliases? From what I can tell with this design
> > > we'll be unable to describe a device on the expansion board that is
> > > wired to properties provided by the two connectors.
> > 
> > Ok, so there are two parts to this.
> > 
> > 1) Allowing a plugin to use multiple connectors.
> > 
> > I thought a bit about this case, but didn't address it for
> > simplicity.  That would require a different syntax, so we can rethink
> > this if it's a use case we think we need.
> 
> Yes it would be nice to design for this case as well.
> 
> > 
> > 2) Dealing with alias collisions between connector types
> > 
> > This one is fairly straightforward to handle.  By default, we'll use
> > labels from connectors we plug into "as is".  However, we can add a
> > syntax that allows us to locally rename labels from a connector (for
> > those familiar with Python, think "import foo from bar as baz").
> > 
> > 
> > So, combining those thoughts together, I'm thinking dtc format for
> > something connecting to two different widget sockets (pretty much the
> > worst case) would look something like:
> > 
> > /plugin/ foo,widget-socket {
> > };
> > 
> > /plugin/ foo,widget-socket {
> >         realias {
> >                 i2c-b = "i2c";
> >                 intc-b = "intc";
> >                 mmio-b = "mmio";
> >         };
> > };
> > 
> > &i2c {
> >         .. devices on the i2c from the first plug ..
> > };
> > 
> > &i2c-b {
> >         .. devices on the i2c from the second plug ..
> > };
> > 
> > Obviously we'd also need to devise an encoding for this to compile
> > into, since the one I proposed previously won't work in this case.
> > 
> 
> I suppose we can distribute the realias nodes when we compile the plugin
> into overlay fragments. The socket matching is a little vague though.
> How would we know which socket to apply to when we have two identical
> looking sockets? I'm thinking we could put some of that information into
> the fragment itself.

So, my assumption in this example was that the plugin could plug into
*any* two widget sockets.  If it needs to connect to specific ones,
then pretty much by definition, the sockets aren't really of
indistinguishable type.

> 
> /{
> 	compatible = "foo,whirlgig-widget";
> 
> 	fragment@0 { /* corresponds to i2c in example above */
> 		target-socket = "foo,widget-socket-a";
> 		target-alias = "i2c";
> 		__overlay__ {
> 			....
> 		};
> 	};
> 
> 	fragment@1 { /* corresponds to i2c-b in example above */
> 		target-socket = "foo,widget-socket-b";
> 		target-alias = "i2c";
> 		__overlay__ {
> 			...
> 		};
> 	};
> };

We don't need any new construct here.  In this case the sockets aren't
100% compatible, which we can notate with
	compatible = "widget-socket-a", "widget-socket";
In the base board.

Devices which can plug into any widget socket will use target-socket =
"widget-socket", those which require a specific one (including
requiring both) can specifically list "widget-socket-a" and/or
"widget-socket-b".

> If we have two identical connectors maybe we'll have to enforce that the
> connectors have some more specific unique compatible string so that we
> can match up the right socket. But I don't see how we can require that
> the overlays know this detail if they only care about one socket and
> could go into either one of them. In that case we should have the loader
> ask the user which socket they connected this extension board to?
> 
> I was also thinking it would be better to leave the gpio-map and
> interrupt-map properties at the connector level. For example:
> 
> 	widget1 {
> 		compatible = "foo,widget-socket";
> 		interrupt-map-mask = <0xffffffff>;
> 		interrupt-map = <0 &intc 7 0>,
> 				<1 &intc 8 0>;
> 	};

That could work - but we should (and implicitly, do) support either
way.  Using subnodes might be useful for particularly complex irq or
gpio mappings.

> and then we could put a label on the plugin/expansion syntax so we can
> reference the connector as a whole:
> 
> 	/plugin/ connector: foo,widget-socket {
> 		compatible = "foo,whirlgig-widget";
> 	};
> 
> 	&i2c {
> 		device@40 {
> 			interrupt-parent = <&connector>;
> 			interrupts = <1>;
> 		};
> 	};
> 
> I also thought about making another alias inside the connector node to
> point to itself, but that fails when you get into the situation of two
> connectors and collisions, unless you rename them of course. It felt
> better to leave that choice to the overlay though.
> 
> In conclusion, I see a few topics/patterns emerging:
> 
> 1) Expose phandles through the connectors in some way that allows us to
>    limit what the plugin/expansion boards can modify or use

Yes, I definitely think we want that.

> 2) Have some flexible syntax to remap cell sizes from the baseboard
>    through the connector to provide a consistent connector size (i.e.
>    remap interrupts and gpios from multiple sources, etc. into a fixed
>    number of cells)

I don't think we need any new constructs here.  If there are
mismatches we can put dummy bridges with appropriate ranges properties
on one side or the other.

The only thing I see that might want some help is that the connector
type should certainly imply a specific set of cell widths for all the
included buses.  So possibly we should supply some stuff to help
enforce that.

> 3) Allow plugin/expansion boards to use multiple connectors from the
>    baseboard in a consistent way

Seems reasonable.

> 4) Attempt to maintain almost all of the current overlay syntax with
>    syntactic sugar

I'm not really sure what you mean by that.

> 5) Connectors on expansion boards should be supported

Again, seems like a good goal to me.

> 
> Is there anything else we should add to this list?
> 

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