Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1445539 > unrolled thread
| Started by | David Gibson <david@gibson.dropbear.id.au> |
|---|---|
| First post | 2016-07-18 16:30 +0200 |
| Last post | 2016-07-22 06:30 +0200 |
| Articles | 13 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-07-18 16:30 +0200
Re: DT connectors, thoughts Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-07-20 23:00 +0200
Re: DT connectors, thoughts Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-07-20 23:10 +0200
Re: DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-07-21 15:50 +0200
Re: DT connectors, thoughts Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-07-21 16:20 +0200
Re: DT connectors, thoughts Rob Herring <robh+dt@kernel.org> - 2016-07-21 21:10 +0200
Re: DT connectors, thoughts Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-07-21 21:20 +0200
Re: DT connectors, thoughts Rob Herring <robh+dt@kernel.org> - 2016-07-21 21:30 +0200
Re: DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-07-22 06:30 +0200
Re: DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-07-22 06:30 +0200
Re: DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-07-22 06:30 +0200
Re: DT connectors, thoughts Rob Herring <robh+dt@kernel.org> - 2016-07-21 21:20 +0200
Re: DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-07-22 06:30 +0200
| From | David Gibson <david@gibson.dropbear.id.au> |
|---|---|
| Date | 2016-07-18 16:30 +0200 |
| Subject | DT connectors, thoughts |
| Message-ID | <rWhIw-7l8-57@gated-at.bofh.it> |
[Multipart message — attachments visible in raw view] — view raw
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;
};
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
>;
};
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>;
};
};
Use of the /plugin/ keyword is rather different from existing
practice, so we may want a new one instead.
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>;
};
};
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?
--
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
[toc] | [next] | [standalone]
| From | Pantelis Antoniou <pantelis.antoniou@konsulko.com> |
|---|---|
| Date | 2016-07-20 23:00 +0200 |
| Message-ID | <rX6KZ-6hA-1@gated-at.bofh.it> |
| In reply to | #1445539 |
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@gibson.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.
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.
In a similar manner we need GPIOs too for every GPIO option on the
connector. Could we fold this in the same node?
> 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/ ?
> 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.
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.
> };
>
>
> 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.
>
> --
> 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
Regards
— Pantelis
[toc] | [prev] | [next] | [standalone]
| From | Pantelis Antoniou <pantelis.antoniou@konsulko.com> |
|---|---|
| Date | 2016-07-20 23:10 +0200 |
| Message-ID | <rX6UG-6zY-7@gated-at.bofh.it> |
| In reply to | #1445539 |
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@gibson.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.
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.
In a similar manner we need GPIOs too for every GPIO option on the
connector. Could we fold this in the same node?
> 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/ ?
> 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.
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.
> };
>
>
> 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.
>
> --
> 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
Regards
— Pantelis
[toc] | [prev] | [next] | [standalone]
| From | David Gibson <david@gibson.dropbear.id.au> |
|---|---|
| Date | 2016-07-21 15:50 +0200 |
| Message-ID | <rXmwp-85l-11@gated-at.bofh.it> |
| In reply to | #1447472 |
[Multipart message — attachments visible in raw view] — view raw
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@gibson.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@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..
> 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@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.
--
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
[toc] | [prev] | [next] | [standalone]
| From | Pantelis Antoniou <pantelis.antoniou@konsulko.com> |
|---|---|
| Date | 2016-07-21 16:20 +0200 |
| Message-ID | <rXmZs-8w7-17@gated-at.bofh.it> |
| In reply to | #1447891 |
Hi David,
> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.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@gibson.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@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
[toc] | [prev] | [next] | [standalone]
| From | Rob Herring <robh+dt@kernel.org> |
|---|---|
| Date | 2016-07-21 21:10 +0200 |
| Message-ID | <rXrw5-3fM-1@gated-at.bofh.it> |
| In reply to | #1447911 |
On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi David,
>
>> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.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@gibson.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.
[...]
>>>> 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.
Interrupt nexus is the interrupt-map property which is fully
supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
for connector gpios, too.
Rob
[toc] | [prev] | [next] | [standalone]
| From | Pantelis Antoniou <pantelis.antoniou@konsulko.com> |
|---|---|
| Date | 2016-07-21 21:20 +0200 |
| Message-ID | <rXrFL-3jF-5@gated-at.bofh.it> |
| In reply to | #1448074 |
Hi Rob,
> On Jul 21, 2016, at 22:09 , Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Hi David,
>>
>>> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.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@gibson.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.
>
> [...]
>
>>>>> 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.
>
> Interrupt nexus is the interrupt-map property which is fully
> supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
> for connector gpios, too.
>
Is interrupt-map enough to cover all our cases? On all the cases that I see it
used is in the context of PCI or some sort of bus.
Is the example above well defined? As far as I can tell interrupt-controller is not
needed.
> Rob
Regards
— Pantelis
[toc] | [prev] | [next] | [standalone]
| From | Rob Herring <robh+dt@kernel.org> |
|---|---|
| Date | 2016-07-21 21:30 +0200 |
| Message-ID | <rXrPs-3ny-17@gated-at.bofh.it> |
| In reply to | #1448082 |
On Thu, Jul 21, 2016 at 2:15 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Jul 21, 2016, at 22:09 , Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Hi David,
>>>
>>>> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.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@gibson.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.
>>
>> [...]
>>
>>>>>> 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.
>>
>> Interrupt nexus is the interrupt-map property which is fully
>> supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
>> for connector gpios, too.
>>
>
> Is interrupt-map enough to cover all our cases? On all the cases that I see it
> used is in the context of PCI or some sort of bus.
I think it should be. IIRC, one of the ARM, Ltd. boards uses it in a
non-PCI context.
> Is the example above well defined? As far as I can tell interrupt-controller is not
> needed.
interrupt-controller should actually be dropped as that is supposed to
be mutually exclusive to interrupt-map, but I think the kernel doesn't
care.
Rob
[toc] | [prev] | [next] | [standalone]
| From | David Gibson <david@gibson.dropbear.id.au> |
|---|---|
| Date | 2016-07-22 06:30 +0200 |
| Message-ID | <rXAg1-R2-9@gated-at.bofh.it> |
| In reply to | #1448082 |
[Multipart message — attachments visible in raw view] — view raw
On Thu, Jul 21, 2016 at 10:15:36PM +0300, Pantelis Antoniou wrote:
> Hi Rob,
>
> > On Jul 21, 2016, at 22:09 , Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
> > <pantelis.antoniou@konsulko.com> wrote:
> >> Hi David,
> >>
> >>> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.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@gibson.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.
> >
> > [...]
> >
> >>>>> 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.
> >
> > Interrupt nexus is the interrupt-map property which is fully
> > supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
> > for connector gpios, too.
> >
>
> Is interrupt-map enough to cover all our cases? On all the cases that I see it
That's the most common use, but it's pretty general. It basically
maps (source irq desc, source unit address) tuples to (dest interrupt
controller, dest irq desc) tuples. The interrupt-map-mask lets us
ignore some parts of that input.
One of the weirder uses was in the DTs for the DMA controller used for
the Ethernet on many PPC 4xx chips. Those had multiple irqs, wired to
different interrupt controllers on some SoCs, but the DT only allows
to specify a single interrupt-parent for a device. We worked around
that by including an interrupt nexus in the node itself, which mapped
a fake local irq number (just an index) to the right irqs and
controllers.
> Is the example above well defined? As far as I can tell interrupt-controller is not
> needed.
The 'interrupt-controller' property should not be there, that was an
error on my part. The rest should be ok.
The plugin piece would need to specify the interrupt-parent as w1_irs
on all its nodes, otherwise they'd just inherit from the parent bus
for their fragment, which might have odd results.
We should probably see if we can figure out a way to enforce that.
--
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
[toc] | [prev] | [next] | [standalone]
| From | David Gibson <david@gibson.dropbear.id.au> |
|---|---|
| Date | 2016-07-22 06:30 +0200 |
| Message-ID | <rXAg1-R2-7@gated-at.bofh.it> |
| In reply to | #1448074 |
[Multipart message — attachments visible in raw view] — view raw
On Thu, Jul 21, 2016 at 02:09:18PM -0500, Rob Herring wrote:
> On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
> > Hi David,
> >
> >> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.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@gibson.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.
>
> [...]
>
> >>>> 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.
>
> Interrupt nexus is the interrupt-map property which is fully
> supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
> for connector gpios, too.
Exactly. I don't know if a gpio-map is already defined, but if it's
not it should be.
--
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
[toc] | [prev] | [next] | [standalone]
| From | David Gibson <david@gibson.dropbear.id.au> |
|---|---|
| Date | 2016-07-22 06:30 +0200 |
| Message-ID | <rXAg1-R2-11@gated-at.bofh.it> |
| In reply to | #1447911 |
[Multipart message — attachments visible in raw view] — view raw
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@gibson.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@gibson.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.
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
[toc] | [prev] | [next] | [standalone]
| From | Rob Herring <robh+dt@kernel.org> |
|---|---|
| Date | 2016-07-21 21:20 +0200 |
| Message-ID | <rXrFL-3jF-9@gated-at.bofh.it> |
| In reply to | #1445539 |
On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
<david@gibson.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;
> };
> 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
> >;
> };
> aliases = {
> i2c = &i2c;
> intc = &w1_irqs;
I understand how you are using i2c alias, but not the intc. It would
help if the same names were not used in multiple places unless they
are the same thing.
What does using aliases here buy us vs. just properties with a phandle?
> 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>;
> };
> };
>
> Use of the /plugin/ keyword is rather different from existing
> practice, so we may want a new one instead.
>
> 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";
Yet another way to express the target... Every new feature for
overlays seems to define a new way. My thinking was the target is a
connector node and all devices are under it. In your case, the
connector is not part of the hierarchy for any devices in the overlay.
That may simplify adding OS support, but seems to be a less accurate
representation of the h/w.
> __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>;
> };
> };
[toc] | [prev] | [next] | [standalone]
| From | David Gibson <david@gibson.dropbear.id.au> |
|---|---|
| Date | 2016-07-22 06:30 +0200 |
| Message-ID | <rXAg1-R2-13@gated-at.bofh.it> |
| In reply to | #1448085 |
[Multipart message — attachments visible in raw view] — view raw
On Thu, Jul 21, 2016 at 02:15:57PM -0500, Rob Herring wrote:
> On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
> <david@gibson.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;
> > };
> > 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
> > >;
> > };
> > aliases = {
> > i2c = &i2c;
> > intc = &w1_irqs;
>
> I understand how you are using i2c alias, but not the intc. It would
> help if the same names were not used in multiple places unless they
> are the same thing.
Yes, sorry. We have both the /soc/intc node which is the base board's
master interrupt controller. Then we have the connector local 'intc'
alias which describes the local interrupt space for just the
connector.
> What does using aliases here buy us vs. just properties with a
> phandle?
Um.. I'm not sure what you mean.
> > 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>;
> > };
> > };
> >
> > Use of the /plugin/ keyword is rather different from existing
> > practice, so we may want a new one instead.
> >
> > 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";
>
> Yet another way to express the target... Every new feature for
> overlays seems to define a new way.
Well, yes. Frankly I think that's because the original ways of
describing the target were not well thought out. Using a phandle is
awkward because it will always be -1 until fixups are applied, which
seems a bit pointless (why not have the alias/label directly). Using
a full path means that the overlay can overwrite anywhere in the base
tree which doesn't seem like good practice.
> My thinking was the target is a
> connector node and all devices are under it.
I thought about this, and I think it's technically possible, but it
gets really ugly. The trouble is that connectors frequently have pins
onto multiple buses. In order to combine those into a single parent
node, we'd have to combine the address spaces of all those buses.
That can be done using some complex multi-cell encoding, but it won't
be pretty. Then to make the connection point be a single node in the
base tree you'd essentially have to combine all the buses used in the
connector throughout the base board DT, so that ugly multi-cell
encoding will have to be used throughout most of the base DT, not just
in the connector stuff.
> In your case, the
> connector is not part of the hierarchy for any devices in the overlay.
> That may simplify adding OS support, but seems to be a less accurate
> representation of the h/w.
I think its the best we can do. Because connectors usually combine
multiple logically distinct buses, they really don't exist as a
well-defined point in a single heirarchy.
> > __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>;
> > };
> > };
>
--
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web