Message-ID: <20160721134208.GD12120@voom.fritz.box>
Date:	Thu, 21 Jul 2016 23:42:08 +1000
From:	David Gibson <david@...son.dropbear.id.au>
To:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
Cc:	Frank Rowand <frowand.list@...il.com>,
	Rob Herring <robh+dt@...nel.org>, stephen.boyd@...aro.org,
	broonie@...nel.org, Grant Likely <grant.likely@...retlab.ca>,
	Mark Rutland <mark.rutland@....com>,
	Matt Porter <mporter@...sulko.com>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Guenter Roeck <linux@...ck-us.net>, marex@...x.de,
	Wolfram Sang <wsa@...-dreams.de>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: DT connectors, thoughts

On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> Spent some time looking at this, and it looks like it’s going to the right direction.
> 
> Comments inline.
> 
> > On Jul 18, 2016, at 17:20 , David Gibson <david@...son.dropbear.id.au> 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;
> > 		};
> 
> MMIO busses are going the way of the dodo and we have serious problems
> handling them in linux in a connector (and a portable manner).
> While we have drivers for GPMC devices we don’t have an in kernel framework
> for handling them.
> 
> A single address range does not contain enough information to program a GPMC interface
> with all the timings and chip select options. It might be possible to declare a
> pre-define memory window on the connector, but it’s use on a real system might
> be limited.

Ok.  I think the example has some value in showing how MMIO ranges and
mapping could be expressed even if it's only part of something more
complex than a simple MMIO bus.

For example I could imagine a connector which includes PCI and some
irq lines.  The PCI part is probable, of course, but a PCI device
wired to one of the hard interrupt lines instead of a PCI interrupt
line would need some DT information.  Of course non-Express, non-MSI
PCI is pretty much extinct too, but it's not much of a stretch to
imagine that something which requires some portion of MMIO mapping is
out there or will come along.

> I think it’s best we focus on standard busses like i2c/spi/i2s/mmc and gpios and
> interrupts for now.
> 
> > 		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
> > 				>;
> > 			};
> 
> This is fine. We need an interrupt controller node.

Actually I think we only need an interrupt nexus, not an interrupt
controller (in IEEE1275 terminology).  (An interrupt controller would
generally require it's own driver, to ack/mask irqs, whereas this just
demonstrates the routing to an existing interrupt controller).  Which
makes that example slightly incorrect (it shouldn't have the
interrupt-controller property).

> In a similar manner we need GPIOs too for every GPIO option on the
> connector. Could we fold this in the same node?

IIRC the GPIO binding is pretty much modeled on the interrupt binding
and has a similar "nexus" concept.  I was expecting the same thing for
GPIO.  It's expressed with different properties to those for irqs,
obviously, so I guess it could be in the same node.  Whether it's
clearer to have them in the same or separate nodes I suspect would
depend on the specifics of the board.

> > 			aliases = {
> > 				i2c = &i2c;
> > 				intc = &w1_irqs;
> > 				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>;
> > 	};
> > };
> > 
> 
> OK, this is brand new syntax. I’m all for it if it makes things easier.
> 
> > Use of the /plugin/ keyword is rather different from existing
> > practice, so we may want a new one instead.
> > 
> 
> It’s a bit weird looking and is bound to cause confusion.
> How about something like /expansion/ ?

That could work.

> > 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";
> > 		__overlay__ {
> > 			whirligig-controller@... {
> > 				...
> > 				interrupt-parent = <0xffffffff>;
> > 				interrupts = <0>;
> > 			};
> > 		};
> > 	};
> > 	__phandle_fixups__ {
> > 		/* These are (path, property, offset) tuples) */
> > 		widget-irqs =
> > 			"/fragment@..._overlay__/whirligig-controller@...",
> > 			"interrupt-parent", <0>;
> > 	};
> 
> I’m not quite sure this is going to work for multiple use of widget-irqs handle,
> but it’s a detail for now.

Just concatenate all the tuples, so path, property, offset, path,
property, offset, etc..

> What is the action undertaken when a bus is activated? Looks like it’s going to
> be similar to my patch where the target/alias bus is given a status=“okay”; property
> and activated, after all subnodes that contain i2c devices are copied there. 

Erm.. what exactly do you mean by "activated"?  At the moment you
could put a status="okay" in the plugin component, and that would be
applied (as long as it goes in one of the accessible attachment
points).

Which does bring up a point.  I did wonder if the approach above
allows the plugin to do too much - e.g. overriding properties in the
i2c controller node, rather than just adding children.  So I did
wonder if we wanted a restriction that only new nodes can be added at
the top level of the plugin fragment.

Alternatively that might be achievable by (as a recommended / best
practice) putting a "container" subnode under each attachable bus on
the master dt and pointing the aliases at that instead of the actual
base bus controller.  With the right 'ranges' etc. that might
accomplish what's needed without extra semantics, but I'm not certain.

Ah.. which makes me think of another point.  In this proposal the
aliases is used to control both where fragments can be attached, and
what nodes can be referenced by phandle.  But we probably want to
split those concepts: e.g. the plugin will need to reference the
interrupt controller / nexus, but probably shouldn't be allowed to
override its properties.

> > };
> > 
> > 
> > Suppose then there's a new version of the board.  This extends the
> > widget socket in a backwards compatible way, but there are now two
> > interchangeable sockets, and they're wired up to different irqs and
> > i2c lines on the baseboard:
> > 
> > /dts-v1/;
> > 
> > / {
> > 	compatible = "foo,newboard";
> > 	ranges;
> > 	soc@... {
> > 		ranges;	
> > 		mmio: mmio-bus@... {
> > 			#address-cells = <2>;
> > 			#size-cells = <2>;
> > 			ranges;
> > 		};
> > 		i2c0: i2c@... {
> > 		};
> > 		i2c1: i2c@... {
> > 		};
> > 		intc: intc@... {
> > 		};
> > 	};
> > 
> > 	connectors {
> > 		widget1 {
> > 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> > 			w1_irqs: irqs {
> > 				interrupt-controller;
> > 				#address-cells = <0>;
> > 				#interrupt-cells = <1>;
> > 				interrupt-map-mask = <0xffffffff>;
> > 				interrupt-map = <
> > 					0 &intc 17 0
> > 					1 &intc 8 0
> > 				>;
> > 			};
> > 			aliases = {
> > 				i2c = &i2c0;
> > 				intc = &w1_irqs;
> > 				mmio = &mmio;
> > 			};
> > 		};
> > 		widget2 {
> > 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> > 			w2_irqs: irqs {
> > 				interrupt-controller;
> > 				#address-cells = <0>;
> > 				#interrupt-cells = <1>;
> > 				interrupt-map-mask = <0xffffffff>;
> > 				interrupt-map = <
> > 					0 &intc 9 0
> > 					1 &intc 10 0
> > 				>;
> > 			};
> > 			aliases = {
> > 				i2c = &i2c1;
> > 				widget-irqs = &w2_irqs;
> > 				mmio = &mmio;
> > 			};
> > 		};
> > 	};
> > };
> > 
> > 
> > A socketed device could also have it's own connectors - the contrived
> > example below has a little 256 byte mmio space (maybe some sort of LPC
> > thingy?):
> > 
> > 
> > /dts-v1/;
> > 
> > /plugin/ foo,widget-socket-v2 {
> > 	compatible = "foo,superduper-widget};
> > 
> > 	connectors {
> > 		compatible = "foo,super-socket";
> > 		aliases {
> > 			superbus = &superbus;
> > 		};	
> > 	};
> > };
> > 
> > &mmio {
> > 	superbus: super-bridge@...000000 {
> > 		#address-cells = <1>;
> > 		#size-cells = <1>;
> > 		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
> > 	};
> > };
> > 
> > &i2c {
> > 	super-controller@... {
> > 		...
> > 	};
> > 	duper-controller@... {
> > 	};
> > };
> > 
> > Thoughts?
> > 
> 
> It’s a step in the right direction, especially if we nail down the
> syntax.

Excellent.

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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists