Re: DT connectors, thoughts

[David Gibson]

On Thu, Jul 21, 2016 at 05:14:33PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Jul 21, 2016, at 16:42 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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@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;
> >>> 		};
> >> 
> >> 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.

So, the kernel shouldn't need the concept of nexus outside the DT
parsing code.  During the DT parsing the kernel will use the
interrupt-map in the nexus to work out which interrupt controllers
each devices are ultimately wired up to.

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

Ok.

> >>> 			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@0/__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.

Hm, shouldn't be that bad - I would have thought it was easier than
string parsing at any rate.  Note that going back to 1275 days there
are existing bindings which define properties with mixed string and
cell data.

But I'm not particularly attached to this format, feel free to suggest
a better one.

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

Ok.  I can see a couple of approaches for that.  One is for the
overlay to set a property in the bus node to mark it as active.
status="okey" is the obvious one but there are other possibilities.
That does raise questions about exactly what the overlay is allowed to
overwrite, of course.

The other possibility is for the overlay to just add the child node
and make sure the driver in the kernel activates the bus or not
depending on whether there are any child devices under it.  This
approach seems like it would be reasonably good practice on the kernel
side anyway, and might have less chance for multiple plugin DTs to
conflict with each other.

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

Ok, probably makes sense to merge those aspects of our two proposals
if we can.

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

...
	i2c@XXX {
		/* i2c controller node */
		device@YYY {
			/* fixed on-board device */
		};
		device@ZZZ {
			/* fixed on-board device */
		};
		socket_i2c: socket-devices {
			ranges;
		};
	};
...

So the idea is the connector description would reference socket_i2c,
rather than the i2c controller itself.  That means that the plugin
can't override properties on the i2c controller, but can add i2c devices.

> > 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@100000000 {
> >>> 		#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

Attachment: signature.asc
Description: PGP signature