[PATCH] powerpc: document new interrupt-array property

[PATCH] powerpc: document new interrupt-array property

[Stuart Yoder]

Added description of an interrupt-array property. This
is needed to cleanly describe the interrupt properties
of devices with interrupts routed to multiple 
interrupt controllers.

Created a new section VII to describe interrupt
representation in general and moved Section VI #2 
('Specifiying interrupt information for SOC devices')
to this new section.

Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
---

 This proposal was based on an offline email exchange with
 Ben and David Gibson.


 Documentation/powerpc/booting-without-of.txt |  149 +++++++++++++++++++-------
 1 files changed, 111 insertions(+), 38 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index eaa0c32..d6d4d09 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -1108,42 +1108,7 @@ See appendix A for an example partial SO
 MPC8540.
 
 
-2) Specifying interrupt information for SOC devices
----------------------------------------------------
-
-Each device that is part of an SOC and which generates interrupts
-should have the following properties:
-
-	- interrupt-parent : contains the phandle of the interrupt
-          controller which handles interrupts for this device
-	- interrupts : a list of tuples representing the interrupt
-          number and the interrupt sense and level for each interrupt
-          for this device.
-
-This information is used by the kernel to build the interrupt table
-for the interrupt controllers in the system.
-
-Sense and level information should be encoded as follows:
-
-   Devices connected to openPIC-compatible controllers should encode
-   sense and polarity as follows:
-
-	0 = low to high edge sensitive type enabled
-	1 = active low level sensitive type enabled
-	2 = active high level sensitive type enabled
-	3 = high to low edge sensitive type enabled
-
-   ISA PIC interrupt controllers should adhere to the ISA PIC
-   encodings listed below:
-
-	0 =  active low level sensitive type enabled
-	1 =  active high level sensitive type enabled
-	2 =  high to low edge sensitive type enabled
-	3 =  low to high edge sensitive type enabled
-
-
-
-3) Representing devices without a current OF specification
+2) Representing devices without a current OF specification
 ----------------------------------------------------------
 
 Currently, there are many devices on SOCs that do not have a standard
@@ -1728,10 +1693,118 @@ platforms are moved over to use the flat
  		partitions = <00000000 00f80000
  			      00f80000 00080001>;
  		partition-names = "fs\0firmware";
- 	};
-
+ 	}; 
    More devices will be defined as this spec matures.
 
+VII - Specifying interrupt information for devices 
+===================================================
+
+The the device tree represents the busses and
+devices of a hardware system in a form similar to the
+physical bus topology of the hardware.
+
+In addition, a logical 'interrupt tree' exists which 
+represents the interrupt routing and topology of the
+hardware.  Devices that generate interrupts have
+a property with a value which is a phandle to the
+parent node in the interrupt tree.  Links between nodes
+in the interrupt tree are from child nodes 'upwards' to 
+their parents and towards the root of the interrupt tree.
+
+The interrupt tree model is fully described in the the
+document "Open Firmware Recommended Practice: Interrupt
+Mapping Version 0.9".  The document is available at:
+http://playground.sun.com/1275/practice.
+
+1) interrupt and interrupt-parent 
+---------------------------------
+
+Devices that generate interrupts to a single interrupt controller
+should use the conventional OF representation described in the
+OF interrupt mapping documentation.  Each device which generates
+interrupts should have the following properties:
+
+	- interrupt-parent : contains the phandle of the interrupt
+          controller which handles interrupts for this device
+          (Note: if interrupt-parent is not specified the interrupt
+           parent is assumed to be the device tree parent)
+	- interrupts : a list of tuples representing the interrupt
+          number and the interrupt sense and level for each interrupt
+          for this device.
+
+This information is used by the kernel to build the interrupt table
+for the interrupt controllers in the system.
+
+Sense and level information should be encoded as follows:
+
+   Devices connected to openPIC-compatible controllers should encode
+   sense and polarity as follows:
+
+	0 = low to high edge sensitive type enabled
+	1 = active low level sensitive type enabled
+	2 = active high level sensitive type enabled
+	3 = high to low edge sensitive type enabled
+
+   ISA PIC interrupt controllers should adhere to the ISA PIC
+   encodings listed below:
+
+	0 =  active low level sensitive type enabled
+	1 =  active high level sensitive type enabled
+	2 =  high to low edge sensitive type enabled
+	3 =  low to high edge sensitive type enabled
+
+2) interrupt-array
+------------------
+
+For devices that generate interrupts to multiple interrupt
+controllers, the interrupts and interrupt-parent representation
+is not sufficient. The interrupt-array property should be used
+to represent this.
+
+The interrupt-array property consists of an arbitrary number
+of 2-tuples consisting of a interrupt parent phandle and
+an interrupt specifier.
+
+  interrupt-array = <parent-phandle0 interrupt-specifier0 ... parent-phandleN interrupt-specifierN>
+
+See the example below:
+
+   pic0: pic@700 {
+       interrupt-controller;
+       #address-cells = <0>;
+       #interrupt-cells = <2>;
+       reg = <700 100>;
+       device_type = "open-pic";
+   };
+
+   pic1: pic@800 {
+       interrupt-controller;
+       #address-cells = <0>;
+       #interrupt-cells = <2>;
+       reg = <800 100>;
+       device_type = "open-pic";
+   };
+
+   ethernet@25000 {
+       #address-cells = <1>;
+       #size-cells = <0>;
+       device_type = "network";
+       model = "TSEC";
+       compatible = "gianfar";
+       reg = <25000 1000>;
+       mac-address = [ 00 E0 0C 00 73 01 ];
+       interrupt-array = <&pic0 13 3 &pic0 14 3 &pic1 18 3>;
+       phy-handle = <2452001>;
+   };
+
+In the example, the interrupt-array defines three
+interrupts-- interrupt 0x13 and 0x14 go to the pic0 interrupt
+controller and interrupt 18 goes to pic1.
+
+Note: the number of cells needed to represent the
+interrupt-specifier is determined by the #interrupt-cells
+property of the interrupt parent.
+
 
 Appendix A - Sample SOC node for MPC8540
 ========================================
-- 
1.4.4

Re: [PATCH] powerpc: document new interrupt-array property

[Kumar Gala]

On Feb 21, 2007, at 5:25 PM, Stuart Yoder wrote:

>
> Added description of an interrupt-array property. This
> is needed to cleanly describe the interrupt properties
> of devices with interrupts routed to multiple
> interrupt controllers.
>
> Created a new section VII to describe interrupt
> representation in general and moved Section VI #2
> ('Specifiying interrupt information for SOC devices')
> to this new section.
>
> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
> ---

what problem is this trying to solve?

- k

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Wed, Feb 21, 2007 at 06:29:22PM -0600, Kumar Gala wrote:
> 
> On Feb 21, 2007, at 5:25 PM, Stuart Yoder wrote:
> 
> >
> > Added description of an interrupt-array property. This
> > is needed to cleanly describe the interrupt properties
> > of devices with interrupts routed to multiple
> > interrupt controllers.
> >
> > Created a new section VII to describe interrupt
> > representation in general and moved Section VI #2
> > ('Specifiying interrupt information for SOC devices')
> > to this new section.
> >
> > Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
> > ---
> 
> what problem is this trying to solve?

As he says, it's for devices with interrupts routed to multiple
controllers.  The normal interrupts property assumes a single
interrupt parent for a device.  This is needed on most 4xx boards,
where the MAL has 5 interrupts, some of which are on UIC0 and some of
which are on UIC1.  At the moment the Ebony port deals with this using
an "interesting" hack, treating the MAL as an interrupt nexus for
itself, using an interrupt-map to remap its interrupts to the two
controllers.  The interrupt-array proposal does the same thing much
more neatly and compactly.

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> what problem is this trying to solve?
>
> As he says, it's for devices with interrupts routed to multiple
> controllers.  The normal interrupts property assumes a single
> interrupt parent for a device.  This is needed on most 4xx boards,
> where the MAL has 5 interrupts, some of which are on UIC0 and some of
> which are on UIC1.  At the moment the Ebony port deals with this using
> an "interesting" hack, treating the MAL as an interrupt nexus for
> itself, using an interrupt-map to remap its interrupts to the two
> controllers.

Not really a hack, this is documented in the interrupt
binding:

> 3.3.  "interrupt-map" property
> At any level in the interrupt tree, a mapping may need to take
> place between the child interrupt domain and the parent=92s.  This
> is represented by a new property called "interrupt-map".

Note it says "*any* level".

> The interrupt-array proposal does the same thing much
> more neatly and compactly.

Sure, for every specific case one can envision a more neat
and compact device tree ;-P

If in a certain tree you have this "problem" with not only
the MAL but with lots of devices, you could introduce a
"fake" interrupt nexus that doesn't represent a physical
device as such, but that represents the combined cascaded
interrupt controllers, and maps the interrupts to the nodes
for the "physical" interrupt controllers.  Just don't make
the mistake of putting an "interrupt-controller" property
in there and all is just fine.  As an added bonus you end up
with one single namespace for the interrupts (one interrupt
domain in interrupt-mapping speak), which is probably what
the chip documentation does as well.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

> Added description of an interrupt-array property. This
> is needed to cleanly describe the interrupt properties
> of devices with interrupts routed to multiple
> interrupt controllers.

I don't really like the idea of adding a new redundant
property to handle this non-problem; but assuming you _do_
really want to add new properties, here are some comments.

> +The the device tree represents the busses and


The the.

> +In addition, a logical 'interrupt tree' exists which
> +represents the interrupt routing and topology of the
> +hardware.

The logical topology only, not necessarily the physical.
And it's not necessarily a tree, you might want to mention
that here, not everyone reads the references :-)

> Devices that generate interrupts have
> +a property with a value which is a phandle to the
> +parent node in the interrupt tree.

Most don't actually, there only is an "interrupt-parent"
property if you don't get to your interrupt parent by
simply walking up in the device tree until you hit an
interrupt controller (possibly applying interrupt maps
along the way).  [All non-root interrupt controllers are
also required to have an "interrupt-parent" property,

> +The interrupt tree model is fully described in the the
> +document "Open Firmware Recommended Practice: Interrupt
> +Mapping Version 0.9".  The document is available at:
> +http://playground.sun.com/1275/practice.

Don't tuck the full stop onto the URL, it makes it hard to
select it.  One good way is to put < > around the URL.

> Each device which generates
> +interrupts should have the following properties:
> +
> +	- interrupt-parent : contains the phandle of the interrupt
> +          controller which handles interrupts for this device
> +          (Note: if interrupt-parent is not specified the interrupt
> +           parent is assumed to be the device tree parent)

Many interrupt devices _shouldn't_ have this property.

> +	- interrupts : a list of tuples representing the interrupt
> +          number and the interrupt sense and level for each interrupt
> +          for this device.

That combination is called an "interrupt specifier", and it
can contain more information (or even different information)
than just number and sense/level.

> +   Devices connected to openPIC-compatible controllers should encode

It's spelled OpenPIC.

> +The interrupt-array property consists of an arbitrary number
> +of 2-tuples consisting of a interrupt parent phandle and
> +an interrupt specifier.

It would be nicer to keep the "interrupts" property and
add a property "interrupt-parents" with the same number
of entries, that encodes the same as "interrupt-parent"
but per interrupt.  You keep more in line with the
"normal" stuff and I suspect it's less code to parse as
well.

> +   ethernet@25000 {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       device_type = "network";
> +       model = "TSEC";

"model" should be more specific, please don't show bad
practice in examples :-)

> +In the example, the interrupt-array defines three
> +interrupts-- interrupt 0x13 and 0x14 go to the pic0 interrupt
> +controller and interrupt 18 goes to pic1.

0x18

> +Note: the number of cells needed to represent the
> +interrupt-specifier is determined by the #interrupt-cells
> +property of the interrupt parent.

Wrong place to document this?  It's true for all interrupt
specifiers.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Thu, Feb 22, 2007 at 08:01:31AM +0100, Segher Boessenkool wrote:
> >> what problem is this trying to solve?
> >
> > As he says, it's for devices with interrupts routed to multiple
> > controllers.  The normal interrupts property assumes a single
> > interrupt parent for a device.  This is needed on most 4xx boards,
> > where the MAL has 5 interrupts, some of which are on UIC0 and some of
> > which are on UIC1.  At the moment the Ebony port deals with this using
> > an "interesting" hack, treating the MAL as an interrupt nexus for
> > itself, using an interrupt-map to remap its interrupts to the two
> > controllers.
> 
> Not really a hack, this is documented in the interrupt
> binding:

No, it really is a hack, I'm afraid.  interrupt-map doesn't in general
make sense for mapping interrupt-children which are not physical
children.  Why?  Because the interrupt map includes unit specifiers,
which means the expected addressing format in the interrupt map must
match that of the reg property in every node mapped through it.

We get away with it in this case because we ignore the unit specifier
part, and the kernel parser happens to use the interrupt parent's
#address-cells value, rather than the physical parent's.

> > 3.3.  "interrupt-map" property
> > At any level in the interrupt tree, a mapping may need to take
> > place between the child interrupt domain and the parent’s.  This
> > is represented by a new property called "interrupt-map".
> 
> Note it says "*any* level".

We could do this properly via an interrupt map by placing the MAL
under a MAL-interrupt-nexus node, which exists to do nothing but remap
the interrupts.  That's pretty horrible, though.

> > The interrupt-array proposal does the same thing much
> > more neatly and compactly.
> 
> Sure, for every specific case one can envision a more neat
> and compact device tree ;-P
> 
> If in a certain tree you have this "problem" with not only
> the MAL but with lots of devices, you could introduce a
> "fake" interrupt nexus that doesn't represent a physical
> device as such, but that represents the combined cascaded
> interrupt controllers, and maps the interrupts to the nodes
> for the "physical" interrupt controllers.  Just don't make
> the mistake of putting an "interrupt-controller" property
> in there and all is just fine.  As an added bonus you end up
> with one single namespace for the interrupts (one interrupt
> domain in interrupt-mapping speak), which is probably what
> the chip documentation does as well.

No, actually.  Afaict for the 4xx chips, the user manuals just give
the interrupt mapping in tables in the the chapter on the interrupt
controllers.  There's a separate table, with separate interrupt
numbers for each UIC.  IIRC, elsewhere in the document the interrupts
are just referred to by name.

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> Not really a hack, this is documented in the interrupt
>> binding:
>
> No, it really is a hack, I'm afraid.  interrupt-map doesn't in general
> make sense for mapping interrupt-children which are not physical
> children.  Why?  Because the interrupt map includes unit specifiers,
> which means the expected addressing format in the interrupt map must
> match that of the reg property in every node mapped through it.

Hrm I guess I misunderstood the way you do things now.
Could you give an example?  I'm too lazy to look up
the DTS file :-)

> We get away with it in this case because we ignore the unit specifier
> part,

That's perfectly fine for many interrupt maps.

> and the kernel parser happens to use the interrupt parent's
> #address-cells value, rather than the physical parent's.

For the child interrupt specifiers, the "#address-cells"
value in the node containing the "interrupt-map" itself
should be used.  For the parent interrupt specifier, the
"#address-cells" value in whatever node the "interrupt-map"
for the matching entry points to should be used.

It sounds like the kernel does the right thing here?

[Of course the #a value better be the same as the value
in the physical parent, and all nodes mapping via a
particular "interrupt-map" better have unique unit
address for that map].

>>> 3.3.  "interrupt-map" property
>>> At any level in the interrupt tree, a mapping may need to take
>>> place between the child interrupt domain and the parent=92s.  This
>>> is represented by a new property called "interrupt-map".
>>
>> Note it says "*any* level".
>
> We could do this properly via an interrupt map by placing the MAL
> under a MAL-interrupt-nexus node, which exists to do nothing but remap
> the interrupts.  That's pretty horrible, though.

It doesn't have to physically be under there, only in the
*interrupt* tree.  This situation isn't specific to your
situation; it happens in other cases with weird wiring
too.

>> As an added bonus you end up
>> with one single namespace for the interrupts (one interrupt
>> domain in interrupt-mapping speak), which is probably what
>> the chip documentation does as well.
>
> No, actually.  Afaict for the 4xx chips, the user manuals just give
> the interrupt mapping in tables in the the chapter on the interrupt
> controllers.  There's a separate table, with separate interrupt
> numbers for each UIC.

Oh okay.


Segher

RE: [PATCH] powerpc: document new interrupt-array property

[Yoder Stuart-B08248]

=20
> -----Original Message-----
> From: linuxppc-dev-bounces+b08248=3Dfreescale.com@ozlabs.org=20
> [mailto:linuxppc-dev-bounces+b08248=3Dfreescale.com@ozlabs.org]=20
> On Behalf Of Segher Boessenkool
> Sent: Thursday, February 22, 2007 5:06 AM
> To: David Gibson
> Cc: linuxppc-dev@ozlabs.org; paulus@samba.org; Yoder Stuart-B08248
> Subject: Re: [PATCH] powerpc: document new interrupt-array property
>=20
> >> Not really a hack, this is documented in the interrupt
> >> binding:
> >
> > No, it really is a hack, I'm afraid.  interrupt-map doesn't=20
> in general
> > make sense for mapping interrupt-children which are not physical
> > children.  Why?  Because the interrupt map includes unit specifiers,
> > which means the expected addressing format in the interrupt map must
> > match that of the reg property in every node mapped through it.
>=20
> Hrm I guess I misunderstood the way you do things now.
> Could you give an example?  I'm too lazy to look up
> the DTS file :-)

So there seems to be 3 options:

Option #1 -- Current 'hack' :) looks like this.  This works but is
ugly.

  MAL0: mcmal {
      /* FIXME */
      device_type =3D "mcmal-dma";
      compatible =3D "ibm,mcmal-440gp", "ibm,mcmal";
      dcr-reg =3D <180 62>;
      num-tx-chans =3D <4>;
      num-rx-chans =3D <4>;
      interrupt-parent =3D <&MAL0>;
      interrupts =3D <0 1 2 3 4>;
      #interrupt-cells =3D <1>;
      #address-cells =3D <0>;
      #size-cells =3D <0>;
      interrupt-map =3D </*TXEOB*/ 0 &UIC0 a 4
               /*RXEOB*/ 1 &UIC0 b 4
               /*SERR*/  2 &UIC1 0 4
               /*TXDE*/  3 &UIC1 1 4
               /*RXDE*/  4 &UIC1 2 4>;
      interrupt-map-mask =3D <ffffffff>;
  };

Option #2 -- new interrupt-array property.  MAL would look
like this:

  MAL0: mcmal {
      /* FIXME */
      device_type =3D "mcmal-dma";
      compatible =3D "ibm,mcmal-440gp", "ibm,mcmal";
      dcr-reg =3D <180 62>;
      num-tx-chans =3D <4>;
      num-rx-chans =3D <4>;
      interrupt-array =3D <&UIC0 a 4
                         &UIC0 b 4
                         &UIC1 0 4
                         &UIC1 1 4
                         &UIC1 2 4>;
  };

Option #3 -- define new, logical interrupt nexus to do
the mapping.   Not sure if I got this right but here is
my take on what this might look like:

  MALINT: malint_nexus {
      #interrupt-cells =3D <1>;
      #address-cells =3D <0>;
      #size-cells =3D <0>;
      interrupt-map =3D </*TXEOB*/ 0 &UIC0 a 4
               /*RXEOB*/ 1 &UIC0 b 4
               /*SERR*/  2 &UIC1 0 4
               /*TXDE*/  3 &UIC1 1 4
               /*RXDE*/  4 &UIC1 2 4>;
      interrupt-map-mask =3D <ffffffff>;
  }

  MAL0: mcmal {
      /* FIXME */
      device_type =3D "mcmal-dma";
      compatible =3D "ibm,mcmal-440gp", "ibm,mcmal";
      dcr-reg =3D <180 62>;
      num-tx-chans =3D <4>;
      num-rx-chans =3D <4>;
      interrupt-parent =3D <&MALINT>;
      interrupts =3D <0 1 2 3 4>;
  };

The malint_nexus node is attache to / I guess??  Segher
is this what you had in mind?

The question is-- is option #3 clear enough?  Is a new
property warranted?

Stuart=20

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

> Option #2 -- new interrupt-array property.  MAL would look
> like this:
>
>   MAL0: mcmal {
>       /* FIXME */
>       device_type = "mcmal-dma";
>       compatible = "ibm,mcmal-440gp", "ibm,mcmal";
>       dcr-reg = <180 62>;
>       num-tx-chans = <4>;
>       num-rx-chans = <4>;
>       interrupt-array = <&UIC0 a 4
>                          &UIC0 b 4
>                          &UIC1 0 4
>                          &UIC1 1 4
>                          &UIC1 2 4>;
>   };

I'd rather write it like

>       interrupts        = < a 4   b 4   0 4   1 4   2 4 >
>       interrupt-parents = <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>

to stay closer to the existing binding, but yes.

> Option #3 -- define new, logical interrupt nexus to do
> the mapping.   Not sure if I got this right but here is
> my take on what this might look like:
>
>   MALINT: malint_nexus {
>       #interrupt-cells = <1>;
>       #address-cells = <0>;
>       #size-cells = <0>;
>       interrupt-map = </*TXEOB*/ 0 &UIC0 a 4
>                /*RXEOB*/ 1 &UIC0 b 4
>                /*SERR*/  2 &UIC1 0 4
>                /*TXDE*/  3 &UIC1 1 4
>                /*RXDE*/  4 &UIC1 2 4>;
>       interrupt-map-mask = <ffffffff>;
>   }
>
>   MAL0: mcmal {
>       /* FIXME */
>       device_type = "mcmal-dma";
>       compatible = "ibm,mcmal-440gp", "ibm,mcmal";
>       dcr-reg = <180 62>;
>       num-tx-chans = <4>;
>       num-rx-chans = <4>;
>       interrupt-parent = <&MALINT>;
>       interrupts = <0 1 2 3 4>;
>   };
>
> The malint_nexus node is attache to / I guess??  Segher
> is this what you had in mind?

I'd put it as a child node of the mcmal node, like this:

   MAL0: mcmal {
       /* FIXME */
       device_type = "mcmal-dma";
       compatible = "ibm,mcmal-440gp", "ibm,mcmal";
       dcr-reg = <180 62>;
       num-tx-chans = <4>;
       num-rx-chans = <4>;
       interrupt-parent = <&MALINT>;
       interrupts = <0 1 2 3 4>;

       MALINT: mal-nexus {
           device_type = "interrupt-nexus"
           #interrupt-cells = <1>;
           interrupt-map = </*TXEOB*/  0   &UIC0   a 4
                            /*RXEOB*/  1   &UIC0   b 4
                            /*SERR*/   2   &UIC1   0 4
                            /*TXDE*/   3   &UIC1   1 4
                            /*RXDE*/   4   &UIC1   2 4>;
           interrupt-map-mask = <ff>;
       }
   }

> The question is-- is option #3 clear enough?

I think so.  A separate nexus pseudo-node needs hardly
any properties (no "#address-cells", no "reg", no nothing)
and nicely expresses the whole thing, within the existing
framework.

> Is a new property warranted?

The proposed "interrupt-array" property, apart from being
a horrible name, only handles one very specific manifestation
of a much more general problem: random weirdness in the
interrupt routing that doesn't conform to the "normal" device
tree.  The existing interrupt mapping binding can handle the
generic case just fine, with a very simple but extremely
flexible framework.  The fact that you sometimes need an
extra "pseudo" device node is actually very natural: it
represent an interrupt "bus".  There is no really good place
for such a node though; depending on the situation, it seems
best to hang it either from the node that represents the
origin of the interrupts (in case this is routing from one
device only), or the node that represents their destination
(typically, an interrupt controller).


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Thu, Feb 22, 2007 at 12:06:09PM +0100, Segher Boessenkool wrote:
> >> Not really a hack, this is documented in the interrupt
> >> binding:
> >
> > No, it really is a hack, I'm afraid.  interrupt-map doesn't in general
> > make sense for mapping interrupt-children which are not physical
> > children.  Why?  Because the interrupt map includes unit specifiers,
> > which means the expected addressing format in the interrupt map must
> > match that of the reg property in every node mapped through it.
> 
> Hrm I guess I misunderstood the way you do things now.
> Could you give an example?  I'm too lazy to look up
> the DTS file :-)
> 
> > We get away with it in this case because we ignore the unit specifier
> > part,
> 
> That's perfectly fine for many interrupt maps.
> 
> > and the kernel parser happens to use the interrupt parent's
> > #address-cells value, rather than the physical parent's.
> 
> For the child interrupt specifiers, the "#address-cells"
> value in the node containing the "interrupt-map" itself
> should be used.  For the parent interrupt specifier, the
> "#address-cells" value in whatever node the "interrupt-map"
> for the matching entry points to should be used.
> 
> It sounds like the kernel does the right thing here?
> 
> [Of course the #a value better be the same as the value
> in the physical parent, and all nodes mapping via a
> particular "interrupt-map" better have unique unit
> address for that map].

Right.  In other words, the addressing format of the interrupt-parent
must match that of the physical parent, which breaks the usual
disconnection between the interrupt tree and the main tree.  Or to
look at it another way, the "reg" property is doubly constrained to
make sense both in the main tree, and in the interrupt tree.  And
really, the only way I can see it working other than by happy
coincidence is if the interrupt-parent *is* the physical parent.

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

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Thu, Feb 22, 2007 at 08:47:28AM -0700, Yoder Stuart-B08248 wrote:
>  
> > -----Original Message-----
> > From: linuxppc-dev-bounces+b08248=freescale.com@ozlabs.org 
> > [mailto:linuxppc-dev-bounces+b08248=freescale.com@ozlabs.org] 
> > On Behalf Of Segher Boessenkool
> > Sent: Thursday, February 22, 2007 5:06 AM
> > To: David Gibson
> > Cc: linuxppc-dev@ozlabs.org; paulus@samba.org; Yoder Stuart-B08248
> > Subject: Re: [PATCH] powerpc: document new interrupt-array property
> > 
> > >> Not really a hack, this is documented in the interrupt
> > >> binding:
> > >
> > > No, it really is a hack, I'm afraid.  interrupt-map doesn't 
> > in general
> > > make sense for mapping interrupt-children which are not physical
> > > children.  Why?  Because the interrupt map includes unit specifiers,
> > > which means the expected addressing format in the interrupt map must
> > > match that of the reg property in every node mapped through it.
> > 
> > Hrm I guess I misunderstood the way you do things now.
> > Could you give an example?  I'm too lazy to look up
> > the DTS file :-)
> 
> So there seems to be 3 options:
> 
> Option #1 -- Current 'hack' :) looks like this.  This works but is
> ugly.
> 
>   MAL0: mcmal {
>       /* FIXME */
>       device_type = "mcmal-dma";
>       compatible = "ibm,mcmal-440gp", "ibm,mcmal";
>       dcr-reg = <180 62>;
>       num-tx-chans = <4>;
>       num-rx-chans = <4>;
>       interrupt-parent = <&MAL0>;
>       interrupts = <0 1 2 3 4>;
>       #interrupt-cells = <1>;
>       #address-cells = <0>;
>       #size-cells = <0>;
>       interrupt-map = </*TXEOB*/ 0 &UIC0 a 4
>                /*RXEOB*/ 1 &UIC0 b 4
>                /*SERR*/  2 &UIC1 0 4
>                /*TXDE*/  3 &UIC1 1 4
>                /*RXDE*/  4 &UIC1 2 4>;
>       interrupt-map-mask = <ffffffff>;
>   };
> 
> Option #2 -- new interrupt-array property.  MAL would look
> like this:
> 
>   MAL0: mcmal {
>       /* FIXME */
>       device_type = "mcmal-dma";
>       compatible = "ibm,mcmal-440gp", "ibm,mcmal";
>       dcr-reg = <180 62>;
>       num-tx-chans = <4>;
>       num-rx-chans = <4>;
>       interrupt-array = <&UIC0 a 4
>                          &UIC0 b 4
>                          &UIC1 0 4
>                          &UIC1 1 4
>                          &UIC1 2 4>;
>   };
> 
> Option #3 -- define new, logical interrupt nexus to do
> the mapping.   Not sure if I got this right but here is
> my take on what this might look like:
> 
>   MALINT: malint_nexus {
>       #interrupt-cells = <1>;
>       #address-cells = <0>;
>       #size-cells = <0>;
>       interrupt-map = </*TXEOB*/ 0 &UIC0 a 4
>                /*RXEOB*/ 1 &UIC0 b 4
>                /*SERR*/  2 &UIC1 0 4
>                /*TXDE*/  3 &UIC1 1 4
>                /*RXDE*/  4 &UIC1 2 4>;
>       interrupt-map-mask = <ffffffff>;
>   }
> 
>   MAL0: mcmal {
>       /* FIXME */
>       device_type = "mcmal-dma";
>       compatible = "ibm,mcmal-440gp", "ibm,mcmal";
>       dcr-reg = <180 62>;
>       num-tx-chans = <4>;
>       num-rx-chans = <4>;
>       interrupt-parent = <&MALINT>;
>       interrupts = <0 1 2 3 4>;
>   };
> 
> The malint_nexus node is attache to / I guess??  Segher
> is this what you had in mind?
> 
> The question is-- is option #3 clear enough?  Is a new
> property warranted?

There's no point to option 3 as given.  If we're going to use an
interrupt nexus, and rely on the fact that the physical versus
interrupt tree addressing mismatch doesn't matter in this case, then
we might as well put the interrupt nexus into the node itself,
i.e. option 1.  The only point to 3 would be if we make the MAL a
child of its interrupt nexus, thereby ensuring that the address forms
match.

Something like:

malint-nexus {
	#interrupt-cells = <1>;
	ranges;
	interrupt-map = <0 0 0 &UIC0 a 4
		.... >;
	interrupt-map-mask = <ffffffff 0 0>;
	MAL0: mcmal {
		device_type = "mcmal-dma";
		compatible = "ibm,mcmal-440gp", "ibm,mcmal";
		dcr-reg = <180 62>;
		num-tx-chans = <4>;
		num-rx-chans = <4>;
		interrupt-parent = <&MALINT>;
		interrupts = <0 1 2 3 4>;
	};
};

Note the empty ranges property (passthrough).  That's kind of
irrelevant here, since MAL is DCR controlled, but would matter if we
had a similar situation with a device that had MMIO registers (and
therefore a "reg" property).  For MAL, since it has no "reg", we set
the interrupt-map-mask to ignore the unit address.

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> Option #3 -- define new, logical interrupt nexus to do
>> the mapping.

> There's no point to option 3 as given.  If we're going to use an
> interrupt nexus, and rely on the fact that the physical versus
> interrupt tree addressing mismatch doesn't matter in this case, then
> we might as well put the interrupt nexus into the node itself,
> i.e. option 1.

That can give problems if there are interrupts in one
of the descendants of the node.  It's also just nasty,
don't you agree?

> The only point to 3 would be if we make the MAL a
> child of its interrupt nexus, thereby ensuring that the address forms
> match.

No, you cannot do that.  There is no extra device
there in reality so it shouldn't be in the device tree
either.  Also, it just doesn't work.

> Something like:
>
> malint-nexus {
> 	#interrupt-cells = <1>;
> 	ranges;
> 	interrupt-map = <0 0 0 &UIC0 a 4
> 		.... >;
> 	interrupt-map-mask = <ffffffff 0 0>;

If you have a "ranges" property you need a
"#address-cells" and a "#size-cells" property
too -- it just doesn't make any sense otherwise.

You don't want this nexus node to be anywhere inside
the "normal" device tree -- it doesn't sit there in
hardware, it shouldn't sit there in the device tree,
that will only cause problems.

> Note the empty ranges property (passthrough).

There is nothing to pass anything through though,
this node shouldn't be here.

> That's kind of
> irrelevant here, since MAL is DCR controlled,

Yeah, so MAL should have the DCR reg in its "reg"
property.  It needs *something* there -- what if
you had two MALs?

> but would matter if we
> had a similar situation with a device that had MMIO registers (and
> therefore a "reg" property).

Sorry, I'm going to shout: "reg" HAS NOTHING TO DO
WITH MMIO.

> For MAL, since it has no "reg", we set
> the interrupt-map-mask to ignore the unit address.

So you're saying your "#address-cells" is not 0, but
you have no "reg" property?  Congratulations, you
built yourself a wildcard package.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> [Of course the #a value better be the same as the value
>> in the physical parent, and all nodes mapping via a
>> particular "interrupt-map" better have unique unit
>> address for that map].
>
> Right.  In other words, the addressing format of the interrupt-parent
> must match that of the physical parent,

Not just the addressing _format_, the _whole_
addressing (addresses need to be unique!)

> which breaks the usual
> disconnection between the interrupt tree and the main tree.

Well they aren't really disconnected anyway.  But yeah I
know what you mean, and it doesn't help constructing
elegant interrupt trees -- you often end up creating
lots of extra nexus nodes, just to get a couple of devices
at the same level.

> Or to
> look at it another way, the "reg" property is doubly constrained to
> make sense both in the main tree, and in the interrupt tree.

But in all cases where it makes sense to use the unit
address in an interrupt map, that map does sit in the
parent node in question so all is good.

> And
> really, the only way I can see it working other than by happy
> coincidence is if the interrupt-parent *is* the physical parent.

It works by having interrupt-controller nodes for
all interrupt controllers, and interrupt-nexus
nodes for all wacky routing / swizzling / whatever.
Since a nexus is a bunch of wires that aren't described
elsewhere in the tree you have to create a node for
it.  It's not physically addressable so you just have to
put it somewhere logical.  In the case of a PCI-PCI
bridge's "standard" swizzling for example, the bridge
node is a reasonable spot.  In many cases there is no
suitable already existing node so you just have to
create a new node.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Fri, Feb 23, 2007 at 01:07:25AM +0100, Segher Boessenkool wrote:
> >> Option #3 -- define new, logical interrupt nexus to do
> >> the mapping.
> 
> > There's no point to option 3 as given.  If we're going to use an
> > interrupt nexus, and rely on the fact that the physical versus
> > interrupt tree addressing mismatch doesn't matter in this case, then
> > we might as well put the interrupt nexus into the node itself,
> > i.e. option 1.
> 
> That can give problems if there are interrupts in one
> of the descendants of the node.  It's also just nasty,
> don't you agree?

Well, if the node has descendents then it's going to need something
more complex in the interrupt-map to handle them, certainly.  But
then, if it has (interrupt) children they'll need to be handled
properly by the nexus in any case.

> > The only point to 3 would be if we make the MAL a
> > child of its interrupt nexus, thereby ensuring that the address forms
> > match.
> 
> No, you cannot do that.  There is no extra device
> there in reality so it shouldn't be in the device tree
> either.  Also, it just doesn't work.

Exactly, there's no extra device in reality.  So I don't see that it's
any more bogus to put the fake device as the MAL's parent than as the
MAL's child or sibling or anywhere else in the tree.

To represent this without either the nexus-in-node or an extra bogus
node, we need something like interrupt-array.  Which is why I like the
idea.

> > Something like:
> >
> > malint-nexus {
> > 	#interrupt-cells = <1>;
> > 	ranges;
> > 	interrupt-map = <0 0 0 &UIC0 a 4
> > 		.... >;
> > 	interrupt-map-mask = <ffffffff 0 0>;
> 
> If you have a "ranges" property you need a
> "#address-cells" and a "#size-cells" property
> too -- it just doesn't make any sense otherwise.

Oh, yes, my mistake.  #a and #s would need to be identical to the
parent of malint-nexus.

> You don't want this nexus node to be anywhere inside
> the "normal" device tree -- it doesn't sit there in
> hardware, it shouldn't sit there in the device tree,
> that will only cause problems.

So why did you suggest its existance in the first place?

> > Note the empty ranges property (passthrough).
> 
> There is nothing to pass anything through though,
> this node shouldn't be here.

If it shouldn't be here, it shouldn't be anywhere else either.

> > That's kind of
> > irrelevant here, since MAL is DCR controlled,
> 
> Yeah, so MAL should have the DCR reg in its "reg"
> property.  It needs *something* there -- what if
> you had two MALs?

No, because that would potentially collide with "reg" properties in
its siblings which have MMIO addresses on the parent bus.

> > but would matter if we
> > had a similar situation with a device that had MMIO registers (and
> > therefore a "reg" property).
> 
> Sorry, I'm going to shout: "reg" HAS NOTHING TO DO
> WITH MMIO.

Not in general, no, but here specifically it does.  "reg" has to match
the address type of the parent bus, in this case that's MMIO.  But the
MAL *is* on the PLB (it's a PLB master, in fact), but it has no
address on the PLB.

> > For MAL, since it has no "reg", we set
> > the interrupt-map-mask to ignore the unit address.
> 
> So you're saying your "#address-cells" is not 0, but
> you have no "reg" property?  Congratulations, you
> built yourself a wildcard package.

How else would you represent a device which exists on the bus, but
which has no address on the bus?

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>>> The only point to 3 would be if we make the MAL a
>>> child of its interrupt nexus, thereby ensuring that the address forms
>>> match.
>>
>> No, you cannot do that.  There is no extra device
>> there in reality so it shouldn't be in the device tree
>> either.  Also, it just doesn't work.
>
> Exactly, there's no extra device in reality.

There is no extra device ***THERE*** in reality, i.e.
between the MAL and the PLB it sits on.  There certainly
_is_ ***AN*** extra device: a bunch of wires routing
different IRQs toi different interrupt controllers.

> So I don't see that it's
> any more bogus to put the fake device as the MAL's parent than as the
> MAL's child or sibling or anywhere else in the tree.

It's not nice to hang random devices at "random" spots
in the tree, but it can't be helped if there is no place
other than in the interrupt tree.  But you simply cannot
make a node that has nothing to do with addressable devices
the parent of a node that *is* an addressable device!

> To represent this without either the nexus-in-node or an extra bogus
> node,

Not bogus.

> we need something like interrupt-array.

Again, the "interrupt-array" helps only this one specific
case, and not the general case *at all*.

> Which is why I like the
> idea.

And I don't :-)

>> You don't want this nexus node to be anywhere inside
>> the "normal" device tree -- it doesn't sit there in
>> hardware, it shouldn't sit there in the device tree,
>> that will only cause problems.
>
> So why did you suggest its existance in the first place?

It is existing hardware.  We need to represent it here.
We just cannot put it anywhere in the tree of addressable
devices since it's not addressable.

> If it shouldn't be here, it shouldn't be anywhere else either.

Bollocks.  If you put the node elsewhere you wouldn't
be pretending that all traffic to/from the MAL goes
through this node.

>>> That's kind of
>>> irrelevant here, since MAL is DCR controlled,
>>
>> Yeah, so MAL should have the DCR reg in its "reg"
>> property.  It needs *something* there -- what if
>> you had two MALs?
>
> No, because that would potentially collide with "reg" properties in
> its siblings which have MMIO addresses on the parent bus.

And since the MAL is not addressable on the PLB it shouldn't
sit on the PLB in the device tree.  It can DMA to the PLB sure,
but DMA isn't represented in the device tree.  The device
tree is the the tree that says "if I want to access this device
what device do I reach it over".

>>> but would matter if we
>>> had a similar situation with a device that had MMIO registers (and
>>> therefore a "reg" property).
>>
>> Sorry, I'm going to shout: "reg" HAS NOTHING TO DO
>> WITH MMIO.
>
> Not in general, no, but here specifically it does.  "reg" has to match
> the address type of the parent bus, in this case that's MMIO.  But the
> MAL *is* on the PLB (it's a PLB master, in fact), but it has no
> address on the PLB.

Yes.  So it's not addressable on the PLB.  So it cannot have
its unit address represent a PLB address.  So it cannot have
the PLB bus as parent.

>>> For MAL, since it has no "reg", we set
>>> the interrupt-map-mask to ignore the unit address.
>>
>> So you're saying your "#address-cells" is not 0, but
>> you have no "reg" property?  Congratulations, you
>> built yourself a wildcard package.
>
> How else would you represent a device which exists on the bus, but
> which has no address on the bus?

If it's not addressable on the bus, it's not represented
on that bus.  Busmasters are represented differently
(typically not at all; in a few cases you need to explicitly
link up devices with IOMMUs).


Segher

RE: [PATCH] powerpc: document new interrupt-array property

[Yoder Stuart-B08248]

=20
> > we need something like interrupt-array.
>=20
> Again, the "interrupt-array" helps only this one specific
> case, and not the general case *at all*.
>=20

Yes-- The interrupt-array helps only this specific case. But
the question is, will this specific case be common in the
future?

If this is a one-off situation it doesn't warrant a new
general property.  However, if it becomes common enough
a simpler representation will benefit developers.  There are
developers who have a hard enough time figuring out the
interrupt-map scheme, much less a logical interrupt nexus
node.

Stuart

Re: [PATCH] powerpc: document new interrupt-array property

[Kumar Gala]

On Feb 23, 2007, at 10:07 AM, Yoder Stuart-B08248 wrote:

>
>>> we need something like interrupt-array.
>>
>> Again, the "interrupt-array" helps only this one specific
>> case, and not the general case *at all*.
>>
>
> Yes-- The interrupt-array helps only this specific case. But
> the question is, will this specific case be common in the
> future?
>
> If this is a one-off situation it doesn't warrant a new
> general property.  However, if it becomes common enough
> a simpler representation will benefit developers.  There are
> developers who have a hard enough time figuring out the
> interrupt-map scheme, much less a logical interrupt nexus
> node.

But how often are developers going to have to tweak these things?   
SoC interrupts tend to be fixed so its just a question of external  
interrupts (and controllers) that a customer has to describe in their  
dts.

- k

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>>> we need something like interrupt-array.
>>
>> Again, the "interrupt-array" helps only this one specific
>> case, and not the general case *at all*.
>>
>
> Yes-- The interrupt-array helps only this specific case. But
> the question is, will this specific case be common in the
> future?

I hope (and think) not.  I can't predict the future though
and there is an approximately 100% chance some hardware will
duplicate this particular weirdness, of course.

> If this is a one-off situation it doesn't warrant a new
> general property.  However, if it becomes common enough
> a simpler representation will benefit developers.

Sure.  But in that case, it would be a lot better to
try to come up with something new that can handle the
general case (or many many more cases at least).

> There are
> developers who have a hard enough time figuring out the
> interrupt-map scheme, much less a logical interrupt nexus
> node.

Adding redundant ways of representing the interrupt
mapping that only apply to minority cases doesn't help
the confusion.  Either educate users how to do things,
come up with a more general more flexible framework,
or both.  Interface explosion only makes the situation
_worse_.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

> But how often are developers going to have to tweak these things?  SoC 
> interrupts tend to be fixed so its just a question of external 
> interrupts (and controllers) that a customer has to describe in their 
> dts.

Yes.  So if we want to add some new representation for
interrupt mapping, it would be good if those users
could benefit.  Then again, I don't see any evidence
that "external users" ever need to do any (complicated)
interrupt mapping stuff.


Segher

RE: [PATCH] powerpc: document new interrupt-array property

[Yoder Stuart-B08248]

> > If this is a one-off situation it doesn't warrant a new
> > general property.  However, if it becomes common enough
> > a simpler representation will benefit developers.
>=20
> Sure.  But in that case, it would be a lot better to
> try to come up with something new that can handle the
> general case (or many many more cases at least).

What is the general case you are referring to here?  What
other cases and scenarios can you imagine?

Stuart

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>>> If this is a one-off situation it doesn't warrant a new
>>> general property.  However, if it becomes common enough
>>> a simpler representation will benefit developers.
>>
>> Sure.  But in that case, it would be a lot better to
>> try to come up with something new that can handle the
>> general case (or many many more cases at least).
>
> What is the general case you are referring to here?

Anything else that currently needs an interrupt nexus
where there is no existing device node to put it in.

> What
> other cases and scenarios can you imagine?

Anything else that currently needs an interrupt nexus
where there is no existing device node to put it in.


Segher

RE: [PATCH] powerpc: document new interrupt-array property

[Yoder Stuart-B08248]

> I'd rather write it like
>=20
> >       interrupts        =3D < a 4   b 4   0 4   1 4   2 4 >
> >       interrupt-parents =3D <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>
>=20

Segher, with your proposal here of an interrupt-parents property
is this really keeping with the normal OF way of representing
property values?

Examples exists where one property tells you how to interpret
or decode another (e.g. #address-cells), but your proposal we
have two distinct properties each with values that together
provide the complete 'value' (interrupt parent + interrupt
specifier).  Is there any precedent for this approach?

I think I'd rather see all the information encoded in
one value.

Stuart

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> I'd rather write it like
>>
>>>       interrupts        = < a 4   b 4   0 4   1 4   2 4 >
>>>       interrupt-parents = <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>
>>
>
> Segher, with your proposal here of an interrupt-parents property
> is this really keeping with the normal OF way of representing
> property values?
>
> Examples exists where one property tells you how to interpret
> or decode another (e.g. #address-cells), but your proposal we
> have two distinct properties each with values that together
> provide the complete 'value' (interrupt parent + interrupt
> specifier).  Is there any precedent for this approach?

"interrupt-parent" normally is a separate property already.
"My" way, you keep the original definition for "interrupts"
and the bleeding obvious definition for "interrupt-parents".

An example where two arrays with corresponding entries is
already used is "alternate-reg" in the PCI binding.  There
are literally hundreds of examples of non-array properties
that only make sense together, of course.

Both "alternate-reg" and "interrupt-parents" can be seen as
an optional extension to their corresponding array properties
("reg" and "interrupts" respectively) so it all makes perfect
sense (to me, at least ;-) ).


Segher

RE: [PATCH] powerpc: document new interrupt-array property

[Yoder Stuart-B08248]

=20

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]=20
> Sent: Friday, February 23, 2007 3:31 PM
> To: Yoder Stuart-B08248
> Cc: David Gibson; linuxppc-dev@ozlabs.org; paulus@samba.org
> Subject: Re: [PATCH] powerpc: document new interrupt-array property
>=20
> >> I'd rather write it like
> >>
> >>>       interrupts        =3D < a 4   b 4   0 4   1 4   2 4 >
> >>>       interrupt-parents =3D <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>
> >>
> >
> > Segher, with your proposal here of an interrupt-parents property
> > is this really keeping with the normal OF way of representing
> > property values?
> >
> > Examples exists where one property tells you how to interpret
> > or decode another (e.g. #address-cells), but your proposal we
> > have two distinct properties each with values that together
> > provide the complete 'value' (interrupt parent + interrupt
> > specifier).  Is there any precedent for this approach?
>=20
> "interrupt-parent" normally is a separate property already.
> "My" way, you keep the original definition for "interrupts"
> and the bleeding obvious definition for "interrupt-parents".
>=20
> An example where two arrays with corresponding entries is
> already used is "alternate-reg" in the PCI binding.  There
> are literally hundreds of examples of non-array properties
> that only make sense together, of course.
>=20
> Both "alternate-reg" and "interrupt-parents" can be seen as
> an optional extension to their corresponding array properties
> ("reg" and "interrupts" respectively) so it all makes perfect
> sense (to me, at least ;-) ).

Here is the difference though-- in the interrupt-parent/interrupts
case you need interrupt-parents to _even parse_ interrupts.

The #interrupt-size could be different for each of the interrupts
parents.

Maybe:
       interrupts        =3D < a 1 4   b 1 4   0 4   1 4   2 4 >
       interrupt-parents =3D <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>

You don't know how many cells per interrupt until you traverse up to
the interrupt parent.

I understand some properties only make sense together, but only
can be parsed together seems ugly.

Stuart

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

> Here is the difference though-- in the interrupt-parent/interrupts
> case you need interrupt-parents to _even parse_ interrupts.

Just like you need the "interrupt-parent" property in
the normal case already.

> The #interrupt-size could be different for each of the interrupts
> parents.

It won't be in most cases though.  And it doesn't _really_
matter.

> I understand some properties only make sense together, but only
> can be parsed together seems ugly.

That is true for many important properties already.  And
it can't be helped unless you a) make the tree way less
flexible; or b) duplicate information all over the place.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[Benjamin Herrenschmidt]

> Sure, for every specific case one can envision a more neat
> and compact device tree ;-P

Still... I do think that even in the general case, providing a way to
directly encode pairs <parent,specifier> is useful, especially when
interrupt are wired in all sort of crazy ways as is common in the
embedded world.
 
> If in a certain tree you have this "problem" with not only
> the MAL but with lots of devices, you could introduce a
> "fake" interrupt nexus that doesn't represent a physical
> device as such, but that represents the combined cascaded
> interrupt controllers, and maps the interrupts to the nodes
> for the "physical" interrupt controllers.  Just don't make
> the mistake of putting an "interrupt-controller" property
> in there and all is just fine.  As an added bonus you end up
> with one single namespace for the interrupts (one interrupt
> domain in interrupt-mapping speak), which is probably what
> the chip documentation does as well.
> 
> 
> Segher
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] powerpc: document new interrupt-array property

[Benjamin Herrenschmidt]

> It would be nicer to keep the "interrupts" property and
> add a property "interrupt-parents" with the same number
> of entries, that encodes the same as "interrupt-parent"
> but per interrupt.  You keep more in line with the
> "normal" stuff and I suspect it's less code to parse as
> well.

There are pro and cons to this approach. I did think about it initially.
The main cons is that "interrupts" becomes harder to parse as you ahve
to walk interrupt-parents at the same time to get the #interrupt-cells
of each entry. Also, it adds more potential for stupid breakage (how
shluld the parser react if interrupt-parents has less entries than
interrupts ?)

The pro, which is quite important too, is that it's a common assuption
that you have interrupts when you have an "interrupts" property, period.
In fact, it would indeed fit a bit better in the current parser.

> Wrong place to document this?  It's true for all interrupt
> specifiers.

Might be worth giving a crazy example where the 2 interrupt specifiers
have different size.

Ben.

RE: [PATCH] powerpc: document new interrupt-array property

[Benjamin Herrenschmidt]

On Fri, 2007-02-23 at 12:15 -0700, Yoder Stuart-B08248 wrote:
> > I'd rather write it like
> > 
> > >       interrupts        = < a 4   b 4   0 4   1 4   2 4 >
> > >       interrupt-parents = <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>
> > 
> 
> Segher, with your proposal here of an interrupt-parents property
> is this really keeping with the normal OF way of representing
> property values?
> 
> Examples exists where one property tells you how to interpret
> or decode another (e.g. #address-cells), but your proposal we
> have two distinct properties each with values that together
> provide the complete 'value' (interrupt parent + interrupt
> specifier).  Is there any precedent for this approach?

Somewhat... interrupt-map and interrupt-map-mask... that sort of thing.

> I think I'd rather see all the information encoded in
> one value.

On the other hand, I do quite like keeping with the old principle that
having interrupts == having an "interrupts" node.

Ben.

RE: [PATCH] powerpc: document new interrupt-array property

[Benjamin Herrenschmidt]

> Maybe:
>        interrupts        = < a 1 4   b 1 4   0 4   1 4   2 4 >
>        interrupt-parents = <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>
> 
> You don't know how many cells per interrupt until you traverse up to
> the interrupt parent.
> 
> I understand some properties only make sense together, but only
> can be parsed together seems ugly.

That's already the case with interrupt-map's anyway. I don't have a firm
preference between the options of interrupt-array and interrupt-parents,
they both have strenghts, though I tend to grow on segher's approach
here.

Ben.

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> It would be nicer to keep the "interrupts" property and
>> add a property "interrupt-parents" with the same number
>> of entries, that encodes the same as "interrupt-parent"
>> but per interrupt.  You keep more in line with the
>> "normal" stuff and I suspect it's less code to parse as
>> well.
>
> There are pro and cons to this approach. I did think about it 
> initially.
> The main cons is that "interrupts" becomes harder to parse as you ahve
> to walk interrupt-parents at the same time to get the #interrupt-cells
> of each entry.

Yeah, you have to do some things inside of the interrupt
parsing loop, that you normally can move outside of the
loop.  That's true in the "interrupt-array" case too, you
just get the interrupt parent from somewhere else.  It's
in the nature of this extension.

> Also, it adds more potential for stupid breakage (how
> shluld the parser react if interrupt-parents has less entries than
> interrupts ?)

Just fail loudly.  Just like it should do if it notes
something else nonsensical (a non-phandle in the
"interrupt-array" case, for example, or running out
of encoded integers while parsing).

> The pro, which is quite important too, is that it's a common assuption
> that you have interrupts when you have an "interrupts" property, 
> period.

Yes, it would be a lot closer to the generic case, only add
to the interrupt mapping binding, not change anything in the
base OF spec.

> In fact, it would indeed fit a bit better in the current parser.

Yeah I think so too.

>> Wrong place to document this?  It's true for all interrupt
>> specifiers.
>
> Might be worth giving a crazy example where the 2 interrupt specifiers
> have different size.

I'd rather not.  Anyway, this was in reply to:

>> +Note: the number of cells needed to represent the
>> +interrupt-specifier is determined by the #interrupt-cells
>> +property of the interrupt parent.

Which is a general statement about interrupt specifiers.
Nowhere does the "booting without OF" doc define interrupt
specifiers before it uses them; do that, move this comment
there, and put a comment saying "each entry in "interrupts"
can potentially have a different # of interrupt cells, you
have to look at the corresponding interrupt parent to find
out" in its place?


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> Sure, for every specific case one can envision a more neat
>> and compact device tree ;-P
>
> Still... I do think that even in the general case, providing a way to
> directly encode pairs <parent,specifier> is useful, especially when
> interrupt are wired in all sort of crazy ways as is common in the
> embedded world.

If you leave the "interrupts" property intact it might indeed
be a simple/clean/compatible enough change to be useful.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> Examples exists where one property tells you how to interpret
>> or decode another (e.g. #address-cells), but your proposal we
>> have two distinct properties each with values that together
>> provide the complete 'value' (interrupt parent + interrupt
>> specifier).  Is there any precedent for this approach?
>
> Somewhat... interrupt-map and interrupt-map-mask... that sort of thing.

Those are not _arrays_ parsed in concert; finding examples
of that is pretty hard (unless you look at PAPR or Apple
trees ;-) ), there are a few though.  Examples where a scalar
property is used to interpret an array property are plenty,
look no further than "interrupt-parent".

>> I think I'd rather see all the information encoded in
>> one value.
>
> On the other hand, I do quite like keeping with the old principle that
> having interrupts == having an "interrupts" node.

Yes, it's a very important principle in OF that any certain
piece of information is encoded in one place, and one possible
place only.

This does mean that you should be very careful when defining
new bindings, think things through a lot to future-proof it,
do a whole lot of peer review, etc.; but in the end you'll end
up with much better bindings.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Sat, Feb 24, 2007 at 07:40:59AM +0100, Benjamin Herrenschmidt wrote:
> On Fri, 2007-02-23 at 12:15 -0700, Yoder Stuart-B08248 wrote:
> > > I'd rather write it like
> > > 
> > > >       interrupts        = < a 4   b 4   0 4   1 4   2 4 >
> > > >       interrupt-parents = <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>
> > > 
> > 
> > Segher, with your proposal here of an interrupt-parents property
> > is this really keeping with the normal OF way of representing
> > property values?
> > 
> > Examples exists where one property tells you how to interpret
> > or decode another (e.g. #address-cells), but your proposal we
> > have two distinct properties each with values that together
> > provide the complete 'value' (interrupt parent + interrupt
> > specifier).  Is there any precedent for this approach?
> 
> Somewhat... interrupt-map and interrupt-map-mask... that sort of thing.
> 
> > I think I'd rather see all the information encoded in
> > one value.
> 
> On the other hand, I do quite like keeping with the old principle that
> having interrupts == having an "interrupts" node.

That would be nice.  On the other hand, re-using interrupts means that
it's possible to get a silent misparse of the interrupt information: a
parser which doesn't understand the new 'interrupt-parents' property
will encounter the node, see the 'interrupts' property, assume that
the interrupt parent is the physical parent and, if the
#interrupt-cells values match up, which is quite possible, assume that
it has correctly understood the interrupt information.

This is arguably a worse behaviour than simply having an old-style
parser see the lack of 'interrupts' property and assume the device has
no interrupts.

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> On the other hand, I do quite like keeping with the old principle that
>> having interrupts == having an "interrupts" node.
>
> That would be nice.  On the other hand, re-using interrupts means that
> it's possible to get a silent misparse of the interrupt information:

Incorrect parsing of interrupt info tends to end up
in spectacular crashes, not silent at all ;-)

> a
> parser which doesn't understand the new 'interrupt-parents' property
> will encounter the node, see the 'interrupts' property, assume that
> the interrupt parent is the physical parent and, if the
> #interrupt-cells values match up, which is quite possible, assume that
> it has correctly understood the interrupt information.

Something similar is true for *every* new binding; although
indeed if you get a misparse the effects can be disastrous,
with interrupts.  For other cases, the kernel would have to
say "I don't understand this device" and give up on it, which
can easily mean a failed boot; or silently assume something
that is just a guess at best.

You cannot boot a client program that doesn't understand the
device tree and expect it to understand the device tree ;-)

> This is arguably a worse behaviour than simply having an old-style
> parser see the lack of 'interrupts' property and assume the device has
> no interrupts.

Until recently (well, not that recently) Linux couldn't
parse the interrupt tree correctly and would royally
mess up in unusual cases.  Does that mean that no device
tree in the world should use the interrupt mapping binding?

Also, a device that has interrupts but no "interrupts"
property is *just wrong*.  There are many more things
that (can) look at the device tree than just the kernel,
don't let your opinion be guided solely by what you
think the kernel would do with a tree.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Mon, Feb 26, 2007 at 06:36:29AM +0100, Segher Boessenkool wrote:
> >> On the other hand, I do quite like keeping with the old principle that
> >> having interrupts == having an "interrupts" node.
> >
> > That would be nice.  On the other hand, re-using interrupts means that
> > it's possible to get a silent misparse of the interrupt information:
> 
> Incorrect parsing of interrupt info tends to end up
> in spectacular crashes, not silent at all ;-)

Well, yes, but "sorry, I can't understand this device tree" or "huh?
I can't find the interrupts" would be preferable to spectacular
crashes.

> > a
> > parser which doesn't understand the new 'interrupt-parents' property
> > will encounter the node, see the 'interrupts' property, assume that
> > the interrupt parent is the physical parent and, if the
> > #interrupt-cells values match up, which is quite possible, assume that
> > it has correctly understood the interrupt information.
> 
> Something similar is true for *every* new binding; although
> indeed if you get a misparse the effects can be disastrous,
> with interrupts.  For other cases, the kernel would have to
> say "I don't understand this device" and give up on it, which
> can easily mean a failed boot; or silently assume something
> that is just a guess at best.
> 
> You cannot boot a client program that doesn't understand the
> device tree and expect it to understand the device tree ;-)

Obviously, but I'd like the client program to *know* that it doesn't
understand the device tree.

> > This is arguably a worse behaviour than simply having an old-style
> > parser see the lack of 'interrupts' property and assume the device has
> > no interrupts.
> 
> Until recently (well, not that recently) Linux couldn't
> parse the interrupt tree correctly and would royally
> mess up in unusual cases.  Does that mean that no device
> tree in the world should use the interrupt mapping binding?
> 
> Also, a device that has interrupts but no "interrupts"
> property is *just wrong*.  There are many more things
> that (can) look at the device tree than just the kernel,
> don't let your opinion be guided solely by what you
> think the kernel would do with a tree.

It's not specific to the kernel, the same reasoning applies to any
program using the device tree.  If something that's not aware of the
new property sees a node with an 'interrupts' but no
'interrupt-parent' property, it has *no reason* to believe there's
anything more to know.

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> Incorrect parsing of interrupt info tends to end up
>> in spectacular crashes, not silent at all ;-)
>
> Well, yes, but "sorry, I can't understand this device tree" or "huh?
> I can't find the interrupts" would be preferable to spectacular
> crashes.

Yes, of course.  It sometimes just can't be helped though.

Oh btw, since Linux has the new interrupt mapping code, you
quite probably will *not* hard crash, the kernel notices the
interrupt map isn't sane and uses a fallback.  You can get
unlucky of course.  Also, and this is just an inherent problem
to all interrupts, many important devices just don't work
without correctly configured interrupts (or their Linux drivers
don't).  With ATA at least you still get one block through
every 30s, but that is hardly optimal ;-)

>> You cannot boot a client program that doesn't understand the
>> device tree and expect it to understand the device tree ;-)
>
> Obviously, but I'd like the client program to *know* that it doesn't
> understand the device tree.

Solving that would be equivalent to the halting problem I'm
afraid.  It can be done for *simple* cases of course.

> It's not specific to the kernel, the same reasoning applies to any
> program using the device tree.  If something that's not aware of the
> new property sees a node with an 'interrupts' but no
> 'interrupt-parent' property, it has *no reason* to believe there's
> anything more to know.

And if a program parsing the device tree sees no valid
"interrupts" property, it can validly assume the device
doesn't have interrupts.

Same problem.

All of this can be avoided by just not defining a new
binding for something as fundamental as interrupt mapping.


Segher

RE: [PATCH] powerpc: document new interrupt-array property

[Yoder Stuart-B08248]

=20

> -----Original Message-----
> From: David Gibson [mailto:david@gibson.dropbear.id.au]=20
> Sent: Sunday, February 25, 2007 10:17 PM
> To: Benjamin Herrenschmidt
> Cc: Yoder Stuart-B08248; linuxppc-dev@ozlabs.org; paulus@samba.org
> Subject: Re: [PATCH] powerpc: document new interrupt-array property
>=20
> On Sat, Feb 24, 2007 at 07:40:59AM +0100, Benjamin=20
> Herrenschmidt wrote:
> > On Fri, 2007-02-23 at 12:15 -0700, Yoder Stuart-B08248 wrote:
> > > > I'd rather write it like
> > > >=20
> > > > >       interrupts        =3D < a 4   b 4   0 4   1 4   2 4 >
> > > > >       interrupt-parents =3D <&UIC0 &UIC0 &UIC1 &UIC1 &UIC1>
> > > >=20
> > >=20
> > > Segher, with your proposal here of an interrupt-parents property
> > > is this really keeping with the normal OF way of representing
> > > property values?
> > >=20
> > > Examples exists where one property tells you how to interpret
> > > or decode another (e.g. #address-cells), but your proposal we
> > > have two distinct properties each with values that together
> > > provide the complete 'value' (interrupt parent + interrupt
> > > specifier).  Is there any precedent for this approach?
> >=20
> > Somewhat... interrupt-map and interrupt-map-mask... that=20
> sort of thing.
> >=20
> > > I think I'd rather see all the information encoded in
> > > one value.
> >=20
> > On the other hand, I do quite like keeping with the old=20
> principle that
> > having interrupts =3D=3D having an "interrupts" node.
>=20
> That would be nice.  On the other hand, re-using interrupts means that
> it's possible to get a silent misparse of the interrupt information: a
> parser which doesn't understand the new 'interrupt-parents' property
> will encounter the node, see the 'interrupts' property, assume that
> the interrupt parent is the physical parent and, if the
> #interrupt-cells values match up, which is quite possible, assume that
> it has correctly understood the interrupt information.
>=20
> This is arguably a worse behaviour than simply having an old-style
> parser see the lack of 'interrupts' property and assume the device has
> no interrupts.

I agree that conceptually this is true.  However, in this particular
case, practically speaking, the only DTS files that would have
this new construct would some new 4xx ones.  If the updates to the
parser
and 4xx board support roughly go in at the same time we shouldn't
have many parser/DTS compatibility issues.

A new property here is really putting some infrastructure in place for
future DTS files.

Stuart

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Mon, Feb 26, 2007 at 03:26:38PM +0100, Segher Boessenkool wrote:
> >> Incorrect parsing of interrupt info tends to end up
> >> in spectacular crashes, not silent at all ;-)
> >
> > Well, yes, but "sorry, I can't understand this device tree" or "huh?
> > I can't find the interrupts" would be preferable to spectacular
> > crashes.
> 
> Yes, of course.  It sometimes just can't be helped though.
> 
> Oh btw, since Linux has the new interrupt mapping code, you
> quite probably will *not* hard crash, the kernel notices the
> interrupt map isn't sane and uses a fallback.  You can get
> unlucky of course.  Also, and this is just an inherent problem
> to all interrupts, many important devices just don't work
> without correctly configured interrupts (or their Linux drivers
> don't).  With ATA at least you still get one block through
> every 30s, but that is hardly optimal ;-)
> 
> >> You cannot boot a client program that doesn't understand the
> >> device tree and expect it to understand the device tree ;-)
> >
> > Obviously, but I'd like the client program to *know* that it doesn't
> > understand the device tree.
> 
> Solving that would be equivalent to the halting problem I'm
> afraid.  It can be done for *simple* cases of course.
> 
> > It's not specific to the kernel, the same reasoning applies to any
> > program using the device tree.  If something that's not aware of the
> > new property sees a node with an 'interrupts' but no
> > 'interrupt-parent' property, it has *no reason* to believe there's
> > anything more to know.
> 
> And if a program parsing the device tree sees no valid
> "interrupts" property, it can validly assume the device
> doesn't have interrupts.
> 
> Same problem.

Sort of.  But the probable consequences of mistakenly believing a
device has no interrupts are substantially less messy than mistakenly
believing you understand the node's interrupts when you don't.

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> And if a program parsing the device tree sees no valid
>> "interrupts" property, it can validly assume the device
>> doesn't have interrupts.
>>
>> Same problem.
>
> Sort of.  But the probable consequences of mistakenly believing a
> device has no interrupts are substantially less messy than mistakenly
> believing you understand the node's interrupts when you don't.

"Less messy"...  well the device won't work properly
in either case.  The kernel might completely screw
up programming the interrupts, which would mean it
doesn't do enough sanity checking; or it could give
spectacular oopses, where the "less messy" case would
simply be a device driver not running for your device.

If the one case gives you more information to track
down the problem than the other case, I argue that's
a shortcoming of the kernel, not of the OF binding.

Anyway, it's all a question of deployment: you just
have to make sure your users use a new enough kernel
with your device tree (and device), which you have
to do *anyway*.  Also DTS files are still conveniently
shipped with the kernel so it's even less of a problem.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Tue, Feb 27, 2007 at 03:52:41AM +0100, Segher Boessenkool wrote:
> >> And if a program parsing the device tree sees no valid
> >> "interrupts" property, it can validly assume the device
> >> doesn't have interrupts.
> >>
> >> Same problem.
> >
> > Sort of.  But the probable consequences of mistakenly believing a
> > device has no interrupts are substantially less messy than mistakenly
> > believing you understand the node's interrupts when you don't.
> 
> "Less messy"...  well the device won't work properly
> in either case.  The kernel might completely screw
> up programming the interrupts, which would mean it
> doesn't do enough sanity checking; or it could give
> spectacular oopses, where the "less messy" case would
> simply be a device driver not running for your device.
> 
> If the one case gives you more information to track
> down the problem than the other case, I argue that's
> a shortcoming of the kernel, not of the OF binding.

Segher, think for a moment instead of just arguing.  There just isn't
enough information available for the kernel to do sanity checking when
there is an apparently valid 'interrupts' property.  Consider:

Interrupt controllers are generally initialized with all interrupts
masked (yes, not always, but usually).  So, if a client mistakenly
believes a device has no interrupts, those interrupts will never be
configured, and the CPU will never see those interrupts.  This is only
going to cause a problem if there is an active driver which is
expecting interrupts.  But if there's a driver expecting interrupts,
it must at some point earlier have attempted to configure the
interrupts (if the client is the kernel, that's a request_irq()).  In
order to configure the interrupt, it would have parsed the device tree
to find data about the interrupt.  In doing so it would have run into
the lack of 'interrupts' property.

There's a good chance at this point it will just print an error saying
"Huh? Where's my interrupt" and abort driver initialization.  If it
doesn't do that, it's very likely it will immediately crash attempting
to dereference or parse the non-existant property.  Either way, the
problem shows up at the point we're attempting to parse the interrupt
tree, and will be pretty easy to debug.

Now, a different case.  Suppose we're using the 'interrupts' /
'interrupt-parents' approach.  We have a board with two identical
interrupt controllers, cascaded.  It has a network device with two
interrupts, the first is end-of-packet and is routed to the top-level
IC, the second signals a fairly rare error condition and is routed to
the cascaded IC.  The network device sits under a bridge which has a
single interrupt routed to the primary IC (and thus has an
'interrupt-parent' property).  So, to an old-style parser it looks
like the network device has two interrupts on the primary controller,
routed via the bridge.

When the network driver initializes, it requests its irqs, correctly
configures the first, and misconfigures the second (because it follows
the interrupt tree old-style and assumes they're all routed to the
primary IC).  It sends and receives packets fine, then the error
condition happens, but the recovery ISR is never called and the
network suddenly stops at some random time after startup.  Programmer,
baffled, tries half-a-dozen theories before noticing the error status
bit and going "but why didn't we get an interrupt?".

Or suppose the second interrupt signals a (fairly unimportant) status
change, level-sensitive.  The network driver works just fine.  Then
along comes another driver that shares an interrupt with the second
network driver interrupt.  It crashes with an unhandled interrupt on
startup if-and-only-if the network driver has had a status change
event before the second driver started.  This is common on some
networks and rare on others.  Bafflement all around...

Or for that matter, the network driver could crash with an unhandled
interrupt when the device which is really using what the network
driver thinks is its second irq, generates an interrupt.  When that
happens could depend on that other device, its driver, the board
configuration, then network or other external environment...

And those are just the first 3 recipes for utter confusion I can come
up.

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

> Segher, think for a moment instead of just arguing.

I do that already, thank you.  I just don't look at
the problem from the same angle as you it seems.

> There just isn't
> enough information available for the kernel to do sanity checking when
> there is an apparently valid 'interrupts' property.  Consider:

It can't do a 100% job (of course; if it could it wouldn't
need a device tree at all since it would be omniscient), but
it still can do quite a reasonable job.

> Interrupt controllers are generally initialized with all interrupts
> masked (yes, not always, but usually).

Yes, and one of the first things I do when debugging interrupt
stuff is this turn all interrupts on since this makes debugging
much much easier.

> So, if a client mistakenly
> believes a device has no interrupts, those interrupts will never be
> configured, and the CPU will never see those interrupts.  This is only
> going to cause a problem if there is an active driver which is
> expecting interrupts.  But if there's a driver expecting interrupts,
> it must at some point earlier have attempted to configure the
> interrupts (if the client is the kernel, that's a request_irq()).  In
> order to configure the interrupt, it would have parsed the device tree
> to find data about the interrupt.  In doing so it would have run into
> the lack of 'interrupts' property.

Yes, give or take some details / ordering of events.

> There's a good chance at this point it will just print an error saying
> "Huh? Where's my interrupt" and abort driver initialization.  If it
> doesn't do that, it's very likely it will immediately crash attempting
> to dereference or parse the non-existant property.

That would be a plain simple bug.

> Either way, the
> problem shows up at the point we're attempting to parse the interrupt
> tree, and will be pretty easy to debug.

Arguably the whole interrupt tree should be parsed at kernel
start, not at request_irq() time.

And this isn't the end of the story; the kernel won't say
"huh where's my IRQ" but it will try a few more options
first (at least on PCI, it can be true on other buses as
well in principle) and quite likely it will return some
bad number.

> Now, a different case.  Suppose we're using the 'interrupts' /
> 'interrupt-parents' approach.  We have a board with two identical
> interrupt controllers, cascaded.  It has a network device with two
> interrupts, the first is end-of-packet and is routed to the top-level
> IC, the second signals a fairly rare error condition and is routed to
> the cascaded IC.  The network device sits under a bridge which has a
> single interrupt routed to the primary IC (and thus has an
> 'interrupt-parent' property).  So, to an old-style parser it looks
> like the network device has two interrupts on the primary controller,
> routed via the bridge.
>
> When the network driver initializes, it requests its irqs, correctly
> configures the first, and misconfigures the second (because it follows
> the interrupt tree old-style and assumes they're all routed to the
> primary IC).

And very likely ends up with a conflict on that second interrupt
since some other device uses it as well.  Stuff will complain
at initialisation time still and all is fine.

> It sends and receives packets fine, then the error
> condition happens, but the recovery ISR is never called and the
> network suddenly stops at some random time after startup.  Programmer,
> baffled, tries half-a-dozen theories before noticing the error status
> bit and going "but why didn't we get an interrupt?".

That would be the programmer who programmed the device tree
(unless he doesn't test his work) so I can't see why he would
be baffled.

Also this is why I like to have all interrupts enabled
on all controllers, you notice the bunch you forgot
about.  If the argument for doing the opposite is "but
it gets really noisy if some unclaimed interrupts happen",
well, that is a very serious bug, right?  Just like you
don't want random DMAs happen, you don't want random
interrupts flying around either.

> Or suppose the second interrupt signals a (fairly unimportant) status
> change, level-sensitive.  The network driver works just fine.  Then
> along comes another driver that shares an interrupt with the second
> network driver interrupt.

It cannot share that interrupt unless the network driver (and
the other device's driver) say it can be shared.  And they
should not, it's level triggered after all.

> It crashes with an unhandled interrupt on
> startup if-and-only-if the network driver has had a status change
> event before the second driver started.  This is common on some
> networks and rare on others.  Bafflement all around...
>
> Or for that matter, the network driver could crash with an unhandled
> interrupt when the device which is really using what the network
> driver thinks is its second irq, generates an interrupt.  When that
> happens could depend on that other device, its driver, the board
> configuration, then network or other external environment...

Yeah, funky interrupt problems are a bitch to resolve,
aren't they.  But the interrupt can't be shared so this
case cannot happen either.

> And those are just the first 3 recipes for utter confusion I can come
> up.

Oh, I can think of many more, and I've seen most of them.

Now back to the meat of the matter:

Whenever you're writing a device tree, after every small
change to the tree you should check it for validity (by
hand or some checking tool), and see if it works on all
kernel versions you claim to support too (not quite the
same thing, heh).  If things go wrong you know what change
caused it.

Some other developer (who didn't change the device tree)
can't run into problems, unless he uses an unsupported
kernel version.  That's his own fault :-)

And, lastly, the most important point that you conveniently
snipped off on your reply:

>> Anyway, it's all a question of deployment: you just
>> have to make sure your users use a new enough kernel
>> with your device tree (and device), which you have
>> to do *anyway*.  Also DTS files are still conveniently
>> shipped with the kernel so it's even less of a problem.

If you care about compatibility to random kernel versions
instead, you'd better not do surgery on the interrupt
mapping binding at all but just put an extra interrupt
nexus in your device tree.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[David Gibson]

On Tue, Feb 27, 2007 at 12:49:26PM +0100, Segher Boessenkool wrote:
> > There just isn't
> > enough information available for the kernel to do sanity checking when
> > there is an apparently valid 'interrupts' property.  Consider:
> 
> It can't do a 100% job (of course; if it could it wouldn't
> need a device tree at all since it would be omniscient), but
> it still can do quite a reasonable job.

Not in at least one common case:  where all interrupt controllers are
of the same type.

> > Interrupt controllers are generally initialized with all interrupts
> > masked (yes, not always, but usually).
> 
> Yes, and one of the first things I do when debugging interrupt
> stuff is this turn all interrupts on since this makes debugging
> much much easier.

All very well once you've realised it's an interrupt problem.

> > So, if a client mistakenly
> > believes a device has no interrupts, those interrupts will never be
> > configured, and the CPU will never see those interrupts.  This is only
> > going to cause a problem if there is an active driver which is
> > expecting interrupts.  But if there's a driver expecting interrupts,
> > it must at some point earlier have attempted to configure the
> > interrupts (if the client is the kernel, that's a request_irq()).  In
> > order to configure the interrupt, it would have parsed the device tree
> > to find data about the interrupt.  In doing so it would have run into
> > the lack of 'interrupts' property.
> 
> Yes, give or take some details / ordering of events.
> 
> > There's a good chance at this point it will just print an error saying
> > "Huh? Where's my interrupt" and abort driver initialization.  If it
> > doesn't do that, it's very likely it will immediately crash attempting
> > to dereference or parse the non-existant property.
> 
> That would be a plain simple bug.

With emphasis on "simple".

> > Either way, the
> > problem shows up at the point we're attempting to parse the interrupt
> > tree, and will be pretty easy to debug.
> 
> Arguably the whole interrupt tree should be parsed at kernel
> start, not at request_irq() time.

Whatever.  At request_irq() time the driver examines the output 

> And this isn't the end of the story; the kernel won't say
> "huh where's my IRQ" but it will try a few more options
> first (at least on PCI, it can be true on other buses as
> well in principle) and quite likely it will return some
> bad number.

Um.. examples?  I can't think of any case except legacy ISA when we'd
have either reason or method to go beyond the device tree information.
Particularly so on SoC or otherwise hardwired embedded devices, which
are the most likely to have this sort of strange interrupt routing.

> > Now, a different case.  Suppose we're using the 'interrupts' /
> > 'interrupt-parents' approach.  We have a board with two identical
> > interrupt controllers, cascaded.  It has a network device with two
> > interrupts, the first is end-of-packet and is routed to the top-level
> > IC, the second signals a fairly rare error condition and is routed to
> > the cascaded IC.  The network device sits under a bridge which has a
> > single interrupt routed to the primary IC (and thus has an
> > 'interrupt-parent' property).  So, to an old-style parser it looks
> > like the network device has two interrupts on the primary controller,
> > routed via the bridge.
> >
> > When the network driver initializes, it requests its irqs, correctly
> > configures the first, and misconfigures the second (because it follows
> > the interrupt tree old-style and assumes they're all routed to the
> > primary IC).
> 
> And very likely ends up with a conflict on that second interrupt
> since some other device uses it as well.  Stuff will complain
> at initialisation time still and all is fine.

Only if the other driver is present and doesn't allow sharing.

> > It sends and receives packets fine, then the error
> > condition happens, but the recovery ISR is never called and the
> > network suddenly stops at some random time after startup.  Programmer,
> > baffled, tries half-a-dozen theories before noticing the error status
> > bit and going "but why didn't we get an interrupt?".
> 
> That would be the programmer who programmed the device tree
> (unless he doesn't test his work) so I can't see why he would
> be baffled.

Not necessarily.  Maybe the error condition never happens in the
device tree programmer's network environment.  The driver seems to
work just fine for him.

> Also this is why I like to have all interrupts enabled
> on all controllers, you notice the bunch you forgot
> about.  If the argument for doing the opposite is "but
> it gets really noisy if some unclaimed interrupts happen",
> well, that is a very serious bug, right?  Just like you
> don't want random DMAs happen, you don't want random
> interrupts flying around either.
> 
> > Or suppose the second interrupt signals a (fairly unimportant) status
> > change, level-sensitive.  The network driver works just fine.  Then
> > along comes another driver that shares an interrupt with the second
> > network driver interrupt.
> 
> It cannot share that interrupt unless the network driver (and
> the other device's driver) say it can be shared.  And they
> should not, it's level triggered after all.

What?  It's perfectly possible to share level triggered interrupts.
PCI devices do it all the time.

> > It crashes with an unhandled interrupt on
> > startup if-and-only-if the network driver has had a status change
> > event before the second driver started.  This is common on some
> > networks and rare on others.  Bafflement all around...
> >
> > Or for that matter, the network driver could crash with an unhandled
> > interrupt when the device which is really using what the network
> > driver thinks is its second irq, generates an interrupt.  When that
> > happens could depend on that other device, its driver, the board
> > configuration, then network or other external environment...
> 
> Yeah, funky interrupt problems are a bitch to resolve,
> aren't they.  But the interrupt can't be shared so this
> case cannot happen either.

Again, yes they can be.

> > And those are just the first 3 recipes for utter confusion I can come
> > up.
> 
> Oh, I can think of many more, and I've seen most of them.
> 
> Now back to the meat of the matter:
> 
> Whenever you're writing a device tree, after every small
> change to the tree you should check it for validity (by
> hand or some checking tool), and see if it works on all
> kernel versions you claim to support too (not quite the
> same thing, heh).  If things go wrong you know what change
> caused it.

Ah, yes, every change should be tested in every configuration.  Lovely
idea, never going to happen.

> Some other developer (who didn't change the device tree)
> can't run into problems, unless he uses an unsupported
> kernel version.  That's his own fault :-)
> 
> And, lastly, the most important point that you conveniently
> snipped off on your reply:
> 
> >> Anyway, it's all a question of deployment: you just
> >> have to make sure your users use a new enough kernel
> >> with your device tree (and device), which you have
> >> to do *anyway*.  Also DTS files are still conveniently
> >> shipped with the kernel so it's even less of a problem.
> 
> If you care about compatibility to random kernel versions
> instead, you'd better not do surgery on the interrupt
> mapping binding at all but just put an extra interrupt
> nexus in your device tree.
> 
> 
> Segher
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

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

Re: [PATCH] powerpc: document new interrupt-array property

[Segher Boessenkool]

>> It can't do a 100% job (of course; if it could it wouldn't
>> need a device tree at all since it would be omniscient), but
>> it still can do quite a reasonable job.
>
> Not in at least one common case:  where all interrupt controllers are
> of the same type.

You'll get a lot of collisions in that case, easy to detect.

>> And this isn't the end of the story; the kernel won't say
>> "huh where's my IRQ" but it will try a few more options
>> first (at least on PCI, it can be true on other buses as
>> well in principle) and quite likely it will return some
>> bad number.
>
> Um.. examples?  I can't think of any case except legacy ISA when we'd
> have either reason or method to go beyond the device tree information.

PCI isn't required to have a device tree at all, with the
flat tree.

>> And very likely ends up with a conflict on that second interrupt
>> since some other device uses it as well.  Stuff will complain
>> at initialisation time still and all is fine.
>
> Only if the other driver is present and doesn't allow sharing.

If _either_ doesn't allow sharing, i.e., almost all of
the time.

>> That would be the programmer who programmed the device tree
>> (unless he doesn't test his work) so I can't see why he would
>> be baffled.
>
> Not necessarily.  Maybe the error condition never happens in the
> device tree programmer's network environment.  The driver seems to
> work just fine for him.

Which just means he didn't do a proper job of testing.

> What?  It's perfectly possible to share level triggered interrupts.
> PCI devices do it all the time.

Erm yeah I goofed, sorry :-)

>> Yeah, funky interrupt problems are a bitch to resolve,
>> aren't they.  But the interrupt can't be shared so this
>> case cannot happen either.
>
> Again, yes they can be.

SoC interrupts shared?  Not very likely...
Sure in principle anything can happen.

>> Now back to the meat of the matter:
>>
>> Whenever you're writing a device tree, after every small
>> change to the tree you should check it for validity (by
>> hand or some checking tool), and see if it works on all
>> kernel versions you claim to support too (not quite the
>> same thing, heh).  If things go wrong you know what change
>> caused it.
>
> Ah, yes, every change should be tested in every configuration.  Lovely
> idea, never going to happen.

One device tree == one configuration.  Static
device trees are easy, heh.

>> And, lastly, the most important point that you conveniently
>> snipped off on your reply:
>>
>>>> Anyway, it's all a question of deployment: you just
>>>> have to make sure your users use a new enough kernel
>>>> with your device tree (and device), which you have
>>>> to do *anyway*.  Also DTS files are still conveniently
>>>> shipped with the kernel so it's even less of a problem.
>>
>> If you care about compatibility to random kernel versions
>> instead, you'd better not do surgery on the interrupt
>> mapping binding at all but just put an extra interrupt
>> nexus in your device tree.

It's still sitting here...

We can have a pointless fight over how hard to debug
certain device tree bugs are, but that's not really
important is it.


Segher

Re: [PATCH] powerpc: document new interrupt-array property

[Benjamin Herrenschmidt]

> PCI isn't required to have a device tree at all, with the
> flat tree.

Yes but if there is a device-node, it's expected somewhat to have proper
interrupt informations (though on PCI they are easy to derive if you
have a correct map in the parent).

Now, we do indeed have some "fallback" code that attempts to find PCI
interrupts using "alternate" methods including reading the
PCI_INTERRUPT_LINE config space entry when the standard parsing fails.
We had to do that to deal with broken firmwares on the field including
old SLOF versions.

> If _either_ doesn't allow sharing, i.e., almost all of
> the time.

Most if not all PCI drivers do allow sharing. It's VERY common to share
PCI interrupts on x86.

> SoC interrupts shared?  Not very likely...
> Sure in principle anything can happen.

SoC generally no. But external interrupts connected to random things on
embedded boards being shared, yes. Heh, look at ARM to see the kind of
crackpot you find on the field (especially in PDAs), it's scary.

Ben.