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 linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
