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. > >> 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@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. >> 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@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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html