Message-Id: <A1018E27-B604-4DD5-ADBA-95EF76F6DDAC@konsulko.com>
Date:	Thu, 21 Jul 2016 17:14:33 +0300
From:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
To:	David Gibson <david@...son.dropbear.id.au>
Cc:	Frank Rowand <frowand.list@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Stephen Boyd <stephen.boyd@...aro.org>,
	Mark Brown <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>,
	Marek Vasut <marex@...x.de>, Wolfram Sang <wsa@...-dreams.de>,
	devicetree <devicetree@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-i2c@...r.kernel.org
Subject: Re: DT connectors, thoughts

Hi David,

> On Jul 21, 2016, at 16:42 , David Gibson <david@...son.dropbear.id.au> wrote:
> 
> 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.
> 

Yep.

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

Yes, this is actually a real system we’re developing against. Non PCI-E
and non-MSI PCI is not extinct, it’s still kicking about, although not
in server/desktop class machines.

We should be able to figure things out.  

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

Hmm, as far as I can tell we only have a concept of an interrupt controller
in the kernel. An interrupt nexus is something new. We should get by without
a driver but hacking the interrupt lookup path at DT.
 
> 
>> 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.
> 

Agreed. I should note that it’s pretty standard for a gpio controller to
advertise itself as an interrupt controller too.

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

Note that parsing that property is going to be really awkward.

It’s [string] [string] [cell], …

We don’t have accessors for something like this.

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

I mean that the bus is by default non-activated. When a portable
connector overlay is applied and the bus is referenced then the
board level bus must be enabled.

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

My RFC patch handles those cases. A connector device declares what kind
of properties are allowed to be copied to the board level bus node
(i.e. clock-freq, speed etc), and whether subnodes are supposed to be
copied there (for i2c client devices etc).

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

Err, I would need an example to grok this.

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

Yep.

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

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

Powered by blists - more mailing lists