[PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Jon Loeliger]

From: Wade Farnsworth <wfarnsworth@mvista.com>

This adds device nodes for the PCI bridges as well as the ISA devices on
the newer revision MPC8641HPCN.  It also adds the PCI ranges to the soc
node so that address translation for the ISA devices works properly.

Includes ULI node originally from Zhang Wei <wei.zhang@freescale.com>.

Signed-off-by: Wade Farnsworth <wfarnsworth@mvista.com>
Signed-off-by: Jon Loeliger <jdl@freescale.com>
---
 arch/powerpc/boot/dts/mpc8641_hpcn.dts |   89 +++++++++++++++++++++++++++----
 1 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8641_hpcn.dts b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
index 260b264..b2a4734 100644
--- a/arch/powerpc/boot/dts/mpc8641_hpcn.dts
+++ b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
@@ -56,7 +56,11 @@
 		#size-cells = <1>;
 		#interrupt-cells = <2>;
 		device_type = "soc";
-		ranges = <0 f8000000 00100000>;
+		ranges = <00000000 f8000000 00100000
+			  80000000 80000000 20000000
+			  e2000000 e2000000 00100000
+			  a0000000 a0000000 20000000
+			  e3000000 e3000000 00100000>;
 		reg = <f8000000 00100000>;	// CCSRBAR 1M
 		bus-frequency = <0>;
 
@@ -285,17 +289,78 @@
 				f800 0 0 3 &i8259 0 0
 				f800 0 0 4 &i8259 0 0
 				>;
-			i8259: i8259@4d0 {
-				clock-frequency = <0>;
-				interrupt-controller;
-				device_type = "interrupt-controller";
-				#address-cells = <0>;
-				#interrupt-cells = <2>;
-				built-in;
-				compatible = "chrp,iic";
-                	        big-endian;
-				interrupts = <49 2>;
-				interrupt-parent = <&mpic>;
+			uli1575@0 {
+				reg = <0 0 0 0 0>;
+				#size-cells = <2>;
+				#address-cells = <3>;
+				ranges = <02000000 0 80000000
+					  02000000 0 80000000
+					  0 20000000
+					  01000000 0 00000000
+					  01000000 0 00000000
+					  0 00100000>;
+
+				pci_bridge@0 {
+					reg = <0 0 0 0 0>;
+					#size-cells = <2>;
+					#address-cells = <3>;
+					ranges = <02000000 0 80000000
+						  02000000 0 80000000
+						  0 20000000
+						  01000000 0 00000000
+						  01000000 0 00000000
+						  0 00100000>;
+
+					isa@1e {
+						device_type = "isa";
+						#interrupt-cells = <2>;
+						#size-cells = <1>;
+						#address-cells = <2>;
+						reg = <f000 0 0 0 0>;
+						ranges = <1 0 01000000 0 0
+							  00001000>;
+						interrupt-parent = <&i8259>;
+
+						i8042@60 {
+							reg = <1 60 1 1 64 1>;
+							interrupts = <1 3 c 3>;
+							interrupt-parent =
+								<&i8259>;
+
+							keyboard@0 {
+								compatible = "pnpPNP,303";
+							};
+
+							mouse@1 {
+								compatible = "pnpPNP,f03";
+							};
+						};
+
+						rtc@70 {
+							compatible =
+								"pnpPNP,b00";
+							reg = <1 70 2>;
+						};
+
+						gpio@400 {
+							reg = <1 400 80>;
+						};
+
+						i8259: i8259@4d0 {
+							clock-frequency = <0>;
+							interrupt-controller;
+							device_type = "interrupt-controller";
+							#address-cells = <0>;
+							#interrupt-cells = <2>;
+							built-in;
+							compatible = "chrp,iic";
+							big-endian;
+							interrupts = <49 2>;
+							interrupt-parent =
+								<&mpic>;
+						};
+					};
+				};
 			};
 
 		};
-- 
1.5.0.3

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

> +		ranges = <00000000 f8000000 00100000
> +			  80000000 80000000 20000000
> +			  e2000000 e2000000 00100000
> +			  a0000000 a0000000 20000000
> +			  e3000000 e3000000 00100000>;
>  		reg = <f8000000 00100000>;	// CCSRBAR 1M


"reg" included in "ranges"?  Something is wrong here.

> +				pci_bridge@0 {

> +					#size-cells = <2>;
> +					#address-cells = <3>;
> +					ranges = <02000000 0 80000000
> +						  02000000 0 80000000
> +						  0 20000000
> +						  01000000 0 00000000
> +						  01000000 0 00000000
> +						  0 00100000>;
> +
> +					isa@1e {
> +						#size-cells = <1>;
> +						#address-cells = <2>;
>
> +						ranges = <1 0 01000000 0 0
> +							  00001000>;

You map the same range (4kB legacy I/O @ 0) for both
bridges here.

> +						i8042@60 {
> +							reg = <1 60 1 1 64 1>;

And this address space is included in both of those.

> +							keyboard@0 {
> +								compatible = "pnpPNP,303";
> +							};
> +
> +							mouse@1 {
> +								compatible = "pnpPNP,f03";
> +							};

These need a "reg" property.

> +						rtc@70 {
> +							compatible =
> +								"pnpPNP,b00";
> +							reg = <1 70 2>;
> +						};
> +
> +						gpio@400 {
> +							reg = <1 400 80>;
> +						};

Inclusive again.

> +						i8259: i8259@4d0 {

Needs "reg".  And 4d0 isn't the primary address
I think?


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Wade Farnsworth]

On Fri, 2007-06-01 at 20:58 +0200, Segher Boessenkool wrote:
> > +		ranges = <00000000 f8000000 00100000
> > +			  80000000 80000000 20000000
> > +			  e2000000 e2000000 00100000
> > +			  a0000000 a0000000 20000000
> > +			  e3000000 e3000000 00100000>;
> >  		reg = <f8000000 00100000>;	// CCSRBAR 1M
> 
> 
> "reg" included in "ranges"?  Something is wrong here.

I think it's correct for soc nodes.  At least, it appears that all of
the dts files with soc nodes do similar things (including this one even
without this patch).

> 
> > +				pci_bridge@0 {
> 
> > +					#size-cells = <2>;
> > +					#address-cells = <3>;
> > +					ranges = <02000000 0 80000000
> > +						  02000000 0 80000000
> > +						  0 20000000
> > +						  01000000 0 00000000
> > +						  01000000 0 00000000
> > +						  0 00100000>;
> > +
> > +					isa@1e {
> > +						#size-cells = <1>;
> > +						#address-cells = <2>;
> >
> > +						ranges = <1 0 01000000 0 0
> > +							  00001000>;
> 
> You map the same range (4kB legacy I/O @ 0) for both
> bridges here.

There is a one-to-one mapping between the I/O spaces of "isa" and
"pci_bridge", so wouldn't it be reasonable that a similar range be used?

> 
> > +						i8042@60 {
> > +							reg = <1 60 1 1 64 1>;
> 
> And this address space is included in both of those.

Again, shouldn't the child's address space be in its parent's range?

> 
> > +							keyboard@0 {
> > +								compatible = "pnpPNP,303";
> > +							};
> > +
> > +							mouse@1 {
> > +								compatible = "pnpPNP,f03";
> > +							};
> 
> These need a "reg" property.

I'll add them.

> 
> > +						rtc@70 {
> > +							compatible =
> > +								"pnpPNP,b00";
> > +							reg = <1 70 2>;
> > +						};
> > +
> > +						gpio@400 {
> > +							reg = <1 400 80>;
> > +						};
> 
> Inclusive again.

See above.

> 
> > +						i8259: i8259@4d0 {
> 
> Needs "reg".  And 4d0 isn't the primary address
> I think?

Yes, this is a standard i8259 with additional registers at 0x20 and
0xa0.  I'll fix the address and add the registers.


--Wade

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> "reg" included in "ranges"?  Something is wrong here.
>
> I think it's correct for soc nodes.

It is not.  "ranges" is the address space translation
between the parent and child busses; "reg" is the stuff
that's on the bridge device itself.

> At least, it appears that all of
> the dts files with soc nodes do similar things (including this one even
> without this patch).

Yes, but that doesn't make it right.

>>> +				pci_bridge@0 {
>>
>>> +					#size-cells = <2>;
>>> +					#address-cells = <3>;
>>> +					ranges = <02000000 0 80000000
>>> +						  02000000 0 80000000
>>> +						  0 20000000
>>> +						  01000000 0 00000000
>>> +						  01000000 0 00000000
>>> +						  0 00100000>;
>>> +
>>> +					isa@1e {
>>> +						#size-cells = <1>;
>>> +						#address-cells = <2>;
>>>
>>> +						ranges = <1 0 01000000 0 0
>>> +							  00001000>;
>>
>> You map the same range (4kB legacy I/O @ 0) for both
>> bridges here.
>
> There is a one-to-one mapping between the I/O spaces of "isa" and
> "pci_bridge", so wouldn't it be reasonable that a similar range be 
> used?

Oh wait, the ISA bridge is a child of the PCI bridge here.
It is obviously fine then.

>>> +						i8042@60 {
>>> +							reg = <1 60 1 1 64 1>;
>>
>> And this address space is included in both of those.
>
> Again, shouldn't the child's address space be in its parent's range?

Yes of course, I'm simply losing track of the nesting
here.  Sigh.

>>> +						i8259: i8259@4d0 {
>>
>> Needs "reg".  And 4d0 isn't the primary address
>> I think?
>
> Yes, this is a standard i8259 with additional registers at 0x20 and
> 0xa0.  I'll fix the address and add the registers.

That sounds good, thanks!


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Gabriel Paubert]

On Sat, Jun 02, 2007 at 10:22:59AM +0200, Segher Boessenkool wrote:
> >> "reg" included in "ranges"?  Something is wrong here.
> >
> > I think it's correct for soc nodes.
> 
> It is not.  "ranges" is the address space translation
> between the parent and child busses; "reg" is the stuff
> that's on the bridge device itself.
> 
> > At least, it appears that all of
> > the dts files with soc nodes do similar things (including this one even
> > without this patch).
> 
> Yes, but that doesn't make it right.
> 
> >>> +				pci_bridge@0 {
> >>
> >>> +					#size-cells = <2>;
> >>> +					#address-cells = <3>;
> >>> +					ranges = <02000000 0 80000000
> >>> +						  02000000 0 80000000
> >>> +						  0 20000000
> >>> +						  01000000 0 00000000
> >>> +						  01000000 0 00000000
> >>> +						  0 00100000>;
> >>> +
> >>> +					isa@1e {
> >>> +						#size-cells = <1>;
> >>> +						#address-cells = <2>;
> >>>
> >>> +						ranges = <1 0 01000000 0 0
> >>> +							  00001000>;
> >>
> >> You map the same range (4kB legacy I/O @ 0) for both
> >> bridges here.
> >
> > There is a one-to-one mapping between the I/O spaces of "isa" and
> > "pci_bridge", so wouldn't it be reasonable that a similar range be 
> > used?
> 
> Oh wait, the ISA bridge is a child of the PCI bridge here.
> It is obviously fine then.
> 
> >>> +						i8042@60 {
> >>> +							reg = <1 60 1 1 64 1>;
> >>
> >> And this address space is included in both of those.
> >
> > Again, shouldn't the child's address space be in its parent's range?
> 
> Yes of course, I'm simply losing track of the nesting
> here.  Sigh.
> 
> >>> +						i8259: i8259@4d0 {
> >>
> >> Needs "reg".  And 4d0 isn't the primary address
> >> I think?
> >
> > Yes, this is a standard i8259 with additional registers at 0x20 and
> > 0xa0.  I'll fix the address and add the registers.
> 
> That sounds good, thanks!
> 

I'd beg to differ. There are three registers area:

- 0x20 which is the master interrupt controller
  (since the original 1981 IBM-PC)
- 0xa0 which is the slave interrupt controller, connected to
  IRQ2 of the master (introduced with the XT or the AT, I don't
  remember)
- 0x4d0 which was added later to allow per interrupt line setting
  of edge or level triggering (instead of per controller).

By far the most important registers are the ones at 0x20 since
you access them at every interrupt. The registers at 0x4d0
are typically set by firmware and never touched later, there
is not a single access to them in sysdev/i8259.c.

I have vague memories of a node named i8259@20 one a board 
who had OF in 1997 (a Motorola MVME but they switched to 
PPCBUG just after :-().

	Gabriel


> 
> Segher
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>>>>> +						i8259: i8259@4d0 {
>>>>
>>>> Needs "reg".  And 4d0 isn't the primary address
>>>> I think?
>>>
>>> Yes, this is a standard i8259 with additional registers at 0x20 and
>>> 0xa0.  I'll fix the address and add the registers.
>>
>> That sounds good, thanks!
>
> I'd beg to differ. There are three registers area:
>
> - 0x20 which is the master interrupt controller
>   (since the original 1981 IBM-PC)
> - 0xa0 which is the slave interrupt controller, connected to
>   IRQ2 of the master (introduced with the XT or the AT, I don't
>   remember)
> - 0x4d0 which was added later to allow per interrupt line setting
>   of edge or level triggering (instead of per controller).
>
> By far the most important registers are the ones at 0x20 since
> you access them at every interrupt. The registers at 0x4d0
> are typically set by firmware and never touched later, there
> is not a single access to them in sysdev/i8259.c.

Yes, I think we all agree -- it should be interrupt-controller@20,
with 20, a0, 4d0 in the "reg" property.

I'm not sure what "compatible" should be for this node, someone
else can dig that up :-)


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Gabriel Paubert]

On Sat, Jun 02, 2007 at 11:01:29AM +0200, Segher Boessenkool wrote:
> >>>>>+						i8259: i8259@4d0 {
> >>>>
> >>>>Needs "reg".  And 4d0 isn't the primary address
> >>>>I think?
> >>>
> >>>Yes, this is a standard i8259 with additional registers at 0x20 and
> >>>0xa0.  I'll fix the address and add the registers.
> >>
> >>That sounds good, thanks!
> >
> >I'd beg to differ. There are three registers area:
> >
> >- 0x20 which is the master interrupt controller
> >  (since the original 1981 IBM-PC)
> >- 0xa0 which is the slave interrupt controller, connected to
> >  IRQ2 of the master (introduced with the XT or the AT, I don't
> >  remember)
> >- 0x4d0 which was added later to allow per interrupt line setting
> >  of edge or level triggering (instead of per controller).
> >
> >By far the most important registers are the ones at 0x20 since
> >you access them at every interrupt. The registers at 0x4d0
> >are typically set by firmware and never touched later, there
> >is not a single access to them in sysdev/i8259.c.
> 
> Yes, I think we all agree -- it should be interrupt-controller@20,
> with 20, a0, 4d0 in the "reg" property.

In the current tree it is called "i8259" in mpc8641_hpcn.dts. The other
ones which have a 8259 are not usable (mpc8555 for example). I don't
even understand their reg property (19000 0 0 0 1), and the driver
has hardcoded addresses at 0x20 and 0xa0, which is reasonable 
since I've never seen an ISA bridge put the 8259 at another address.

> 
> I'm not sure what "compatible" should be for this node, someone
> else can dig that up :-)

I believe that "8259" should appear somewhere because of the 
"8259-interrupt-acknowledge" property (defined in CHRP bindings)
which you can have on the parent bridge to speed up interrupt 
vector acquisition.

	Gabriel

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> Yes, I think we all agree -- it should be interrupt-controller@20,
>> with 20, a0, 4d0 in the "reg" property.
>
> In the current tree it is called "i8259" in mpc8641_hpcn.dts.

"interrupt-controller" is the preferred name for all
interrupt controller nodes.  The "compatible" property
can easily distinguish between different types.  Not
that it matters all that much -- just remember that
the "name" is used for device matching before "compatible"
is (_should_ be, Linux ignores this part of the standard
right now in most cases), so don't put junk in there!

> The other
> ones which have a 8259 are not usable (mpc8555 for example). I don't
> even understand their reg property (19000 0 0 0 1),

Very weird indeed.  What bustype is this?  (I looked it up,
it's PCI, and I can't make heads or tails of it either --
it says it sits on bus #1, although its parent isn't bus #1;
and the final "1" should be "0".  Many more weird things
in that node, too).

> and the driver
> has hardcoded addresses at 0x20 and 0xa0, which is reasonable
> since I've never seen an ISA bridge put the 8259 at another address.

As long as the driver (platform code I hope?) at least checks
for the existence of the node, that's fair enough I guess.
Actually using the "reg" would be better of course.

>> I'm not sure what "compatible" should be for this node, someone
>> else can dig that up :-)

Oh what the hell, I'm too curious...  "pnpPNP,0" it is.

> I believe that "8259" should appear somewhere because of the
> "8259-interrupt-acknowledge" property (defined in CHRP bindings)
> which you can have on the parent bridge to speed up interrupt
> vector acquisition.

You're not CHRP so you have nothing to do with the CHRP
bindings...


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Sat, 2007-06-02 at 22:23 +0200, Segher Boessenkool wrote:
> 
> 
> Oh what the hell, I'm too curious...  "pnpPNP,0" it is.
> 
> > I believe that "8259" should appear somewhere because of the
> > "8259-interrupt-acknowledge" property (defined in CHRP bindings)
> > which you can have on the parent bridge to speed up interrupt
> > vector acquisition.
> 
> You're not CHRP so you have nothing to do with the CHRP
> bindings...

Still, you are close to CHRP and the CHRP bindings do apply :-) the 8259
should thus be chrp,iic :-) It's even documented that way in
booting-without-of.txt iirc.

Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> Oh what the hell, I'm too curious...  "pnpPNP,0" it is.
>>
>>> I believe that "8259" should appear somewhere because of the
>>> "8259-interrupt-acknowledge" property (defined in CHRP bindings)
>>> which you can have on the parent bridge to speed up interrupt
>>> vector acquisition.
>>
>> You're not CHRP so you have nothing to do with the CHRP
>> bindings...
>
> Still, you are close to CHRP

Not at all, the rest of the device interrupt subsystem
is very different, too.

> and the CHRP bindings do apply :-)

Nope.

> the 8259
> should thus be chrp,iic :-) It's even documented that way in
> booting-without-of.txt iirc.

So fix the docs :-)


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Gabriel Paubert]

On Sun, Jun 03, 2007 at 09:41:33AM +0200, Segher Boessenkool wrote:
> >>Oh what the hell, I'm too curious...  "pnpPNP,0" it is.
> >>
> >>>I believe that "8259" should appear somewhere because of the
> >>>"8259-interrupt-acknowledge" property (defined in CHRP bindings)
> >>>which you can have on the parent bridge to speed up interrupt
> >>>vector acquisition.
> >>
> >>You're not CHRP so you have nothing to do with the CHRP
> >>bindings...
> >
> >Still, you are close to CHRP
> 
> Not at all, the rest of the device interrupt subsystem
> is very different, too.

Given how well you agree, I understand how other people
might get a bit confused ;-)

This said, I'm looking at device trees right now, and I can understand
that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
I don't understand at all why it is &pci1 on the mpc85??cds.dts.

But the definition of the ISA bridge in these files is very strange to 
start with: I've never seen an ISA bridge with only an interrupt controller 
on it, no interrupts are connected to it and its reg property is
almost certainly wrong. Maybe it is an example of things that should
not be done.


> >and the CHRP bindings do apply :-)
> 
> Nope.

At least the 8259-interrupt-acknowledge property of the bus
on which the ISA bridge is found should keep its name. It's what
the kernel uses.

It just seems odd to call the chip iic (for ISA interrupt controller 
I suppose) on one side and 8259 on the other. Naming consistency
is important.

Technically the 8259-interrupt-acknowledge is not a very fortunate
name choise since it is an a 8259 like chip (or an ISA bridge 
for the matter) that has to claim the interrupt acknowledge
cycles, but it is what happens in practice. 

> 
> >the 8259
> >should thus be chrp,iic :-) It's even documented that way in
> >booting-without-of.txt iirc.

Nope.

	Gabriel

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>>>> Oh what the hell, I'm too curious...  "pnpPNP,0" it is.
>>>>
>>>>> I believe that "8259" should appear somewhere because of the
>>>>> "8259-interrupt-acknowledge" property (defined in CHRP bindings)
>>>>> which you can have on the parent bridge to speed up interrupt
>>>>> vector acquisition.
>>>>
>>>> You're not CHRP so you have nothing to do with the CHRP
>>>> bindings...
>>>
>>> Still, you are close to CHRP
>>
>> Not at all, the rest of the device interrupt subsystem
>> is very different, too.
>
> Given how well you agree, I understand how other people
> might get a bit confused ;-)

Heh.  Let's look at it some other way: if you use
"chrp,iic", this node needs to be handled by either
CHRP platform code (which will not work, you aren't
CHRP), or by platform-specific code.  If you use the
generic "pnpPNP,0" name, it can (in theory) be handled
by architecture-neutral code, even.

For the foreseeable future (or perhaps forever) the
interrupt stuff will be set up from the platform
code, so there is no direct impact on Linux.  You
might want to support other OSes, or be more future-
proof, or generically want a cleaner device tree.

> This said, I'm looking at device trees right now, and I can understand
> that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
> I don't understand at all why it is &pci1 on the mpc85??cds.dts.

Perhaps the 8259 IRQ output is routed to a PCI
interrupt.  If not, this is just plain wrong.

> But the definition of the ISA bridge in these files is very strange to
> start with: I've never seen an ISA bridge with only an interrupt 
> controller
> on it, no interrupts are connected to it

Maybe that's why it works ;-)

> and its reg property is
> almost certainly wrong.

<19000 0 0 0 0> could be right, that means device d#18
on PCI bus 1.  There is a missing bus #1 node in between
though.  And the unit address is @12 in that case,
not @19000 .

> Maybe it is an example of things that should
> not be done.

Maybe :-)

>>> and the CHRP bindings do apply :-)
>>
>> Nope.
>
> At least the 8259-interrupt-acknowledge property of the bus
> on which the ISA bridge is found should keep its name. It's what
> the kernel uses.

Only the CHRP Linux platform code should use that.
It's not necessarily a bad idea to define your
platform's binding to do the same, of course --
other platform's bindings do not apply to yours,
but there is no harm in creating a certain similarity.

I have no idea what this whole 8259-ack thing is
so I cannot comment further.

> It just seems odd to call the chip iic (for ISA interrupt controller
> I suppose) on one side and 8259 on the other. Naming consistency
> is important.

It certainly helps, yes.

> Technically the 8259-interrupt-acknowledge is not a very fortunate
> name choise since it is an a 8259 like chip (or an ISA bridge
> for the matter) that has to claim the interrupt acknowledge
> cycles, but it is what happens in practice.

Could you explain what this is (or point me to
some doc)?

>>> the 8259
>>> should thus be chrp,iic :-) It's even documented that way in
>>> booting-without-of.txt iirc.
>
> Nope.

Good to hear :-)


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Sun, 2007-06-03 at 10:57 +0200, Segher Boessenkool wrote:
> 
> I have no idea what this whole 8259-ack thing is
> so I cannot comment further.

When you get an IRQ on your main PIC (MPIC for example) which happens to
be the cascade to the 8259, you have two ways of retreiving the actual
8259 interrupt source.

One is the "poll" method which goes read IOs on the 8259. The other one
is to generate a PCI interrupt acknowledge cycle on the bus that leads
to the 8259. If your bridges properly forward it all the way to the ISA
bridge, it should "mimminc" the x86 interrupt acknowledge and return the
interrupt number (an INTACK cycle on PCI is pretty much a read from a
broadcast address).

The later is more efficient but the mecanism to generate such cycles is
very much platform specific and was never abstracted in the PCI stack
(possibly because x86 never needs to explicitely do it, it's all
implicit in the x86 bus protocol :-)

So on CHRP, that property in a PHB indicates, when possible, and address
you can ioremap and readb from to generate an INTACK cycle on that bus
and retreive the pending IRQ of any legacy PIC on that segment.

In theory, in fact, MPIC itself, at least the PCI variant of it, is also
supposed to be able to respond to these rather than reading an MMIO
register (remember, MPIC was supposed to be useable on x86 too, though I
don't know if that was never actually implemented). But then, I've never
seen that used or implemented in practice.

Cheers,
Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> I have no idea what this whole 8259-ack thing is
>> so I cannot comment further.
>

> When you get an IRQ on your main PIC (MPIC for example) which happens 
> to
> be the cascade to the 8259, you have two ways of retreiving the actual
> 8259 interrupt source.
>
> One is the "poll" method which goes read IOs on the 8259. The other one
> is to generate a PCI interrupt acknowledge cycle on the bus that leads
> to the 8259. If your bridges properly forward it all the way to the ISA
> bridge, it should "mimminc" the x86 interrupt acknowledge and return 
> the
> interrupt number (an INTACK cycle on PCI is pretty much a read from a
> broadcast address).

Ah, that stuff.

> So on CHRP, that property in a PHB indicates, when possible, and 
> address
> you can ioremap and readb from to generate an INTACK cycle on that bus
> and retreive the pending IRQ of any legacy PIC on that segment.

Right, so it is just misnamed (and belongs in the "reg"
property for that specific PHB anyway).  It has nothing
to do with 8259, it is part of the PCI spec.

> In theory, in fact, MPIC itself, at least the PCI variant of it, is 
> also
> supposed to be able to respond to these rather than reading an MMIO
> register (remember, MPIC was supposed to be useable on x86 too, though 
> I
> don't know if that was never actually implemented).

OpenPIC was I think.  No idea if it ever actually got
used anywhere though.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Gabriel Paubert]

On Sun, Jun 03, 2007 at 10:57:59AM +0200, Segher Boessenkool wrote:
> >>>>Oh what the hell, I'm too curious...  "pnpPNP,0" it is.
> >>>>
> >>>>>I believe that "8259" should appear somewhere because of the
> >>>>>"8259-interrupt-acknowledge" property (defined in CHRP bindings)
> >>>>>which you can have on the parent bridge to speed up interrupt
> >>>>>vector acquisition.
> >>>>
> >>>>You're not CHRP so you have nothing to do with the CHRP
> >>>>bindings...
> >>>
> >>>Still, you are close to CHRP
> >>
> >>Not at all, the rest of the device interrupt subsystem
> >>is very different, too.
> >
> >Given how well you agree, I understand how other people
> >might get a bit confused ;-)
> 
> Heh.  Let's look at it some other way: if you use
> "chrp,iic", this node needs to be handled by either
> CHRP platform code (which will not work, you aren't
> CHRP), or by platform-specific code.  If you use the
> generic "pnpPNP,0" name, it can (in theory) be handled
> by architecture-neutral code, even.
> 

Thanks for this explanation.

> For the foreseeable future (or perhaps forever) the
> interrupt stuff will be set up from the platform
> code, so there is no direct impact on Linux.  You
> might want to support other OSes, or be more future-
> proof, or generically want a cleaner device tree.
> 
> >This said, I'm looking at device trees right now, and I can understand
> >that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
> >I don't understand at all why it is &pci1 on the mpc85??cds.dts.
> 
> Perhaps the 8259 IRQ output is routed to a PCI
> interrupt.  If not, this is just plain wrong.

Maybe, but it ultimately has to go the mpic, no?
I can see how the cascade of interrupt parents and 
other specifications could lead there.

> 
> >But the definition of the ISA bridge in these files is very strange to
> >start with: I've never seen an ISA bridge with only an interrupt 
> >controller
> >on it, no interrupts are connected to it
> 
> Maybe that's why it works ;-)

That maybe the best explanation.

> 
> >and its reg property is
> >almost certainly wrong.
> 
> <19000 0 0 0 0> could be right, that means device d#18
> on PCI bus 1.  There is a missing bus #1 node in between
> though.  And the unit address is @12 in that case,
> not @19000 .
> 
> >Maybe it is an example of things that should
> >not be done.
> 
> Maybe :-)
> 
> >>>and the CHRP bindings do apply :-)
> >>
> >>Nope.
> >
> >At least the 8259-interrupt-acknowledge property of the bus
> >on which the ISA bridge is found should keep its name. It's what
> >the kernel uses.
> 
> Only the CHRP Linux platform code should use that.
> It's not necessarily a bad idea to define your
> platform's binding to do the same, of course --
> other platform's bindings do not apply to yours,
> but there is no harm in creating a certain similarity.
> 
> I have no idea what this whole 8259-ack thing is
> so I cannot comment further.

It is a way to fetch the vector from the interrupt controller.
Among all the types of cycles defined by PCI (configuration,
I/O and memory spaces read and write), there is one which 
is generated by the host bridge when an x86 processor 
acknowledges an interrupt (it has changed with the APIC
and IO-APIC where "A" stands for awkward IMO[1]).

The sequence of events is the following:
- an interrupt is raised
- when switching to the interrupt handler, the
  processor generates a special bus cycle
- this is translated by the host bridge into
  a PCI interrupt acknowledge cycle
- one and only one agent on the bus behind the host bridge
  (this cycle is not propagated by P2P bridges) answers
  by placing a byte sized interrupt vector on the bus.
- this vector is used by the processor to index into the 
  interrupt descriptor table and starts execution wherever
  it is told to go.

Generating a pci interrupt acknowledge cycle is a faster 
way to get at the vector than doing this explicitly in
the controller: a simple byte fetch instead of 2 or
4 I/O accesses guardered by a lock acquisition and
release.

If you need more details, ask me.

Now a question: how would you describe the nvram and RTC
on the PreP boards that I have?

In the residual data I have:

ISA Device, Slot 0, LogicalDev 0: IBM0008, SystemPeripheral, NVRAM, #-1, IndirectNVRAM
  Device flags 2800: Integrated, Static
  Packets describing allocated resources:
    Variable (16 decoded bits) I/O port
      from 0x0074 to 0x0074, alignment 1, 2 ports
    Variable (16 decoded bits) I/O port
      from 0x0077 to 0x0077, alignment 1, 1 ports
  No packets describing possible resources.
  No packets describing compatible resources.
ISA Device, Slot 0, LogicalDev 0: PNP0B00, SystemPeripheral, RealTimeClock, #-1, interface 129
  Device flags 2800: Integrated, Static
  Packets describing allocated resources:
    Variable (16 decoded bits) I/O port
      from 0x0074 to 0x0074, alignment 1, 2 ports
    Variable (16 decoded bits) I/O port
      from 0x0077 to 0x0077, alignment 1, 1 ports
    Chip identification: MOT3040
    Small vendor item type 0x00, data (hex): 01 f8 1f 00 00 
  No packets describing possible resources.
  No packets describing compatible resources.

It is the same chip that has the NVRAM and the RTC, only that 
the last 8 bytes of the NVRAM change with time. I've not found
the last "Small vendor item" described in any doc, but it looks
like a byte of 1, followed by the offset of the RTC inside the 
NVRAM in little-endian byte order.

But essentially the problem is that I have two logically 
different devices in the same chip. Of course the size of
the nvram should be somewhere (8k-8, starting at 0).


	Gabriel


[1] Read the last paragraph of http://lkml.org/lkml/2006/11/1/294, 
and I believe that it is an understatement. There are also Linus'
rants about handling of edge triggered interrupts in APIC: it's
fairly easy to lose them, that's what cutting-edge (pun intended)
innovation is :-)

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>>> This said, I'm looking at device trees right now, and I can 
>>> understand
>>> that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
>>> I don't understand at all why it is &pci1 on the mpc85??cds.dts.
>>
>> Perhaps the 8259 IRQ output is routed to a PCI
>> interrupt.  If not, this is just plain wrong.
>
> Maybe, but it ultimately has to go the mpic, no?

Yes, certainly.  A PowerPC CPU has only one connection
to one interrupt controller, everything else hangs below
that one.

>> I have no idea what this whole 8259-ack thing is
>> so I cannot comment further.
>
> It is a way to fetch the vector from the interrupt controller.

[big snip]

Thanks, I did know that stuff, just not the device
tree property :-)

But a great explanation anyway.

> If you need more details, ask me.

That was quite detailed enough, heh :-)

> Now a question: how would you describe the nvram and RTC
> on the PreP boards that I have?
>
> In the residual data I have:
>
> ISA Device, Slot 0, LogicalDev 0: IBM0008, SystemPeripheral, NVRAM, 
> #-1, IndirectNVRAM
>   Device flags 2800: Integrated, Static
>   Packets describing allocated resources:
>     Variable (16 decoded bits) I/O port
>       from 0x0074 to 0x0074, alignment 1, 2 ports
>     Variable (16 decoded bits) I/O port
>       from 0x0077 to 0x0077, alignment 1, 1 ports

This is an IBM NVRAM thing -- write address to ISA I/O
0x74/0x75, read/write a byte from 0x76.

This node should be a child of the "isa" (or isa
compatible) bus, and look something like this:

nvram@i74 {
	device_type = "nvram";
	regs = <1 74 3>;
	compatible = "whatever-chip-this-is";
	#bytes = <2000>; // 8kB, just an example
}

> ISA Device, Slot 0, LogicalDev 0: PNP0B00, SystemPeripheral, 
> RealTimeClock, #-1, interface 129
>   Device flags 2800: Integrated, Static
>   Packets describing allocated resources:
>     Variable (16 decoded bits) I/O port
>       from 0x0074 to 0x0074, alignment 1, 2 ports
>     Variable (16 decoded bits) I/O port
>       from 0x0077 to 0x0077, alignment 1, 1 ports
>     Chip identification: MOT3040
>     Small vendor item type 0x00, data (hex): 01 f8 1f 00 00

The I/O port numbers here are wrong.

rtc@i70 {
	device_type = "rtc";
	reg = <1 70 2>;
	compatible = "pnpPNP,b00";
}

You might want to set the register range to 4 in both
cases, esp. if the chip actually decodes those ranges;
there are no useful registers there though (AFAIK).

> It is the same chip that has the NVRAM and the RTC, only that
> the last 8 bytes of the NVRAM change with time. I've not found
> the last "Small vendor item" described in any doc, but it looks
> like a byte of 1, followed by the offset of the RTC inside the
> NVRAM in little-endian byte order.
>
> But essentially the problem is that I have two logically
> different devices in the same chip.

That isn't a problem -- logically they are both children
of the ISA bus, just describe them like that.  This is
done all the time, just look at some SoC tree ;-)

> Of course the size of
> the nvram should be somewhere (8k-8, starting at 0).

It's implicit from the chip type; the Device Support
Extensions recommended practice defines the "#bytes"
property for nvram devices though.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Gabriel Paubert]

On Sun, Jun 03, 2007 at 01:42:22PM +0200, Segher Boessenkool wrote:
> >>>This said, I'm looking at device trees right now, and I can 
> >>>understand
> >>>that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
> >>>I don't understand at all why it is &pci1 on the mpc85??cds.dts.
> >>
> >>Perhaps the 8259 IRQ output is routed to a PCI
> >>interrupt.  If not, this is just plain wrong.
> >
> >Maybe, but it ultimately has to go the mpic, no?
> 
> Yes, certainly.  A PowerPC CPU has only one connection
> to one interrupt controller, everything else hangs below
> that one.
> 
> >>I have no idea what this whole 8259-ack thing is
> >>so I cannot comment further.
> >
> >It is a way to fetch the vector from the interrupt controller.
> 
> [big snip]
> 
> Thanks, I did know that stuff, just not the device
> tree property :-)
> 
> But a great explanation anyway.
> 
> >If you need more details, ask me.
> 
> That was quite detailed enough, heh :-)
> 
> >Now a question: how would you describe the nvram and RTC
> >on the PreP boards that I have?
> >
> >In the residual data I have:
> >
> >ISA Device, Slot 0, LogicalDev 0: IBM0008, SystemPeripheral, NVRAM, 
> >#-1, IndirectNVRAM
> >  Device flags 2800: Integrated, Static
> >  Packets describing allocated resources:
> >    Variable (16 decoded bits) I/O port
> >      from 0x0074 to 0x0074, alignment 1, 2 ports
> >    Variable (16 decoded bits) I/O port
> >      from 0x0077 to 0x0077, alignment 1, 1 ports
> 
> This is an IBM NVRAM thing -- write address to ISA I/O
> 0x74/0x75, read/write a byte from 0x76.

Data is 0x77 actually. Port 0x76 systematically returns 0xff
on this board (I have one at hand right now and am doing
accesses with the firmware).

> 
> This node should be a child of the "isa" (or isa
> compatible) bus, and look something like this:
> 
> nvram@i74 {
> 	device_type = "nvram";
> 	regs = <1 74 3>;
> 	compatible = "whatever-chip-this-is";
> 	#bytes = <2000>; // 8kB, just an example
> }
> 
> >ISA Device, Slot 0, LogicalDev 0: PNP0B00, SystemPeripheral, 
> >RealTimeClock, #-1, interface 129
> >  Device flags 2800: Integrated, Static
> >  Packets describing allocated resources:
> >    Variable (16 decoded bits) I/O port
> >      from 0x0074 to 0x0074, alignment 1, 2 ports
> >    Variable (16 decoded bits) I/O port
> >      from 0x0077 to 0x0077, alignment 1, 1 ports
> >    Chip identification: MOT3040
> >    Small vendor item type 0x00, data (hex): 01 f8 1f 00 00
> 
> The I/O port numbers here are wrong.

No they aren't. It uses exactly the same port as for the NVRAM.

Otherwise the "feature" of this RTC is that its interrupt
is not connected. 

> 
> rtc@i70 {
> 	device_type = "rtc";
> 	reg = <1 70 2>;
> 	compatible = "pnpPNP,b00";
> }
> 

Nope, there is nothing at 0x70-0x71 (read returns 0xff). The chip
is a 48T59. Look in prep_setup.c in arch/ppc/platforms.
Motorola and IBM boards use different RTC. 

The lines: 

	} else {
		TODC_INIT(TODC_TYPE_MK48T59, PREP_NVRAM_AS0, PREP_NVRAM_AS1,
				PREP_NVRAM_DATA, 8);
	}

show that it uses the same addresses as the nvram. Actually I wonder 
whether using PNP0B00 is correct in the residual data here. 

	Gabriel

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> This is an IBM NVRAM thing -- write address to ISA I/O
>> 0x74/0x75, read/write a byte from 0x76.
>
> Data is 0x77 actually. Port 0x76 systematically returns 0xff
> on this board (I have one at hand right now and am doing
> accesses with the firmware).

Oh okay, I don't know exactly what chip this is so
obviously my information isn't 100% ;-)

>>> ISA Device, Slot 0, LogicalDev 0: PNP0B00, SystemPeripheral,
>>> RealTimeClock, #-1, interface 129
>>>  Device flags 2800: Integrated, Static
>>>  Packets describing allocated resources:
>>>    Variable (16 decoded bits) I/O port
>>>      from 0x0074 to 0x0074, alignment 1, 2 ports
>>>    Variable (16 decoded bits) I/O port
>>>      from 0x0077 to 0x0077, alignment 1, 1 ports
>>>    Chip identification: MOT3040
>>>    Small vendor item type 0x00, data (hex): 01 f8 1f 00 00
>>
>> The I/O port numbers here are wrong.
>
> No they aren't. It uses exactly the same port as for the NVRAM.

Then its claim to be PNP 0b00 is incorrect.

> Otherwise the "feature" of this RTC is that its interrupt
> is not connected.

Dunno what you mean here?

>> rtc@i70 {
>> 	device_type = "rtc";
>> 	reg = <1 70 2>;
>> 	compatible = "pnpPNP,b00";
>> }
>>
>
> Nope, there is nothing at 0x70-0x71 (read returns 0xff). The chip
> is a 48T59.

[Not so easy to find a datasheet for that -- STM M48T59Y
is what I found in the end]

This chip doesn't sit on any I/O port range, it is 8kB
of direct-mapped standard SRAM stuff.  There must be
some latches or such on your board, or perhaps this is
driven via some superio chip or something like that.

> Actually I wonder
> whether using PNP0B00 is correct in the residual data here.

It's not correct at all, no.

Not sure how best to describe this thing -- one master
node with a kid for both nvram and rtc; one node; or
perhaps one node for rtc, with a child node for nvram.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Gabriel Paubert]

On Sun, Jun 03, 2007 at 04:42:38PM +0200, Segher Boessenkool wrote:
> >>This is an IBM NVRAM thing -- write address to ISA I/O
> >>0x74/0x75, read/write a byte from 0x76.
> >
> >Data is 0x77 actually. Port 0x76 systematically returns 0xff
> >on this board (I have one at hand right now and am doing
> >accesses with the firmware).
> 
> Oh okay, I don't know exactly what chip this is so
> obviously my information isn't 100% ;-)
> 
> >>>ISA Device, Slot 0, LogicalDev 0: PNP0B00, SystemPeripheral,
> >>>RealTimeClock, #-1, interface 129
> >>> Device flags 2800: Integrated, Static
> >>> Packets describing allocated resources:
> >>>   Variable (16 decoded bits) I/O port
> >>>     from 0x0074 to 0x0074, alignment 1, 2 ports
> >>>   Variable (16 decoded bits) I/O port
> >>>     from 0x0077 to 0x0077, alignment 1, 1 ports
> >>>   Chip identification: MOT3040
> >>>   Small vendor item type 0x00, data (hex): 01 f8 1f 00 00
> >>
> >>The I/O port numbers here are wrong.
> >
> >No they aren't. It uses exactly the same port as for the NVRAM.
> 
> Then its claim to be PNP 0b00 is incorrect.
> 
> >Otherwise the "feature" of this RTC is that its interrupt
> >is not connected.
> 
> Dunno what you mean here?

On a standard PC, the RTC is connected to interrupt
8 of the 8259 pair. On these boards the interrupt output
of the RTC is not connected (there is no interrupt
in the list of properties). Interrupt 8 is connected
to a front panel button labeled "ABT".

> 
> >>rtc@i70 {
> >>	device_type = "rtc";
> >>	reg = <1 70 2>;
> >>	compatible = "pnpPNP,b00";
> >>}
> >>
> >
> >Nope, there is nothing at 0x70-0x71 (read returns 0xff). The chip
> >is a 48T59.
> 
> [Not so easy to find a datasheet for that -- STM M48T59Y
> is what I found in the end]

It is the correct one.

> 
> This chip doesn't sit on any I/O port range, it is 8kB
> of direct-mapped standard SRAM stuff.  There must be
> some latches or such on your board, or perhaps this is
> driven via some superio chip or something like that.

Indeed, the latches are a bit hard to find (they are inside a 
Lattice chip), and the data byte is directly connected to the 
ISA bus of the PIB (W83C553 / W53C554).

On some variants of the board, there is not even a superIO chip
(only a discrete UART, TL16C550 or similar).

> 
> >Actually I wonder
> >whether using PNP0B00 is correct in the residual data here.
> 
> It's not correct at all, no.

Ok. I don't know well the PNP spec. I don't really remember
also what the chip identification means in the residual data,
but it is always MOT3040. OTOH, there is a but in more
recent versions, that claims that the NVRAM data size is
32768 and not 8192...

> 
> Not sure how best to describe this thing -- one master
> node with a kid for both nvram and rtc; one node; or
> perhaps one node for rtc, with a child node for nvram.
> 

Hey, at least you now understand why I was asking the question :-)
This is the only device on these boards that really causes
me trouble, for all the others I think I can get a reasonable
description in the device tree. 

	Gabriel

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>>> Otherwise the "feature" of this RTC is that its interrupt
>>> is not connected.
>>
>> Dunno what you mean here?
>
> On a standard PC, the RTC is connected to interrupt
> 8 of the 8259 pair.

Ah okay.  This RTC is a completely different device
though.  It does have an interrupt line, but it might
very well not be connected anywhere.

>>> Actually I wonder
>>> whether using PNP0B00 is correct in the residual data here.
>>
>> It's not correct at all, no.
>
> Ok. I don't know well the PNP spec.

b00 means PC-AT style RTC.

> I don't really remember
> also what the chip identification means in the residual data,
> but it is always MOT3040.

I searched for that too, but couldn't find anything else
than Linux-on-PReP related stuff ;-)

> OTOH, there is a but in more
> recent versions, that claims that the NVRAM data size is
> 32768 and not 8192...

You can fix that in the device tree :-)

>> Not sure how best to describe this thing -- one master
>> node with a kid for both nvram and rtc; one node; or
>> perhaps one node for rtc, with a child node for nvram.
>
> Hey, at least you now understand why I was asking the question :-)

Yeah, it's a bit nasty case.  Since it _is_ one device,
you really want to describe it as such -- but it also
is nice to describe the nvram functionality separately.
I think I'd go for

rtc@i74 {
	device_type = "rtc";
	reg = <1 74 4>;
	compatible = "MK42whatever";

	nvram {
		device_type = "nvram";
		#bytes = <1ff0>;
	}
}

or something like that.  You'll have to do the device
matching as one device, but the driver has to know they
are one device *anyway* :-)

> This is the only device on these boards that really causes
> me trouble, for all the others I think I can get a reasonable
> description in the device tree.

Great to hear, looking forward to seeing it.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

> > Not at all, the rest of the device interrupt subsystem
> > is very different, too.
> 
> Given how well you agree, I understand how other people
> might get a bit confused ;-)

Well... we initially decided to base the whole linux device-tree thing
on the CHRP specification. After all, it's flexible and pretty much
everything that has a PCI bus (and especially if it has an i8259) can
almost be considered as a superset of CHRP :-)

Now, Segher seems to disagree, but I tend to think that since it's been
the common practice -and- the chrp spec to make the cascaded i8259 on
the ISA bridge "chrp,iic" so far, doing differently gratuituously will
not help anybody whatsoever.

But feel free to disagree, it's not of terrible importance. The board
code is the one to "find" the PICs, including the cascaded 8259, the
acutal 8259 driver takes whatever node is given to it, so as long as
your .dts matches your board, it doesn't matter that much.

> This said, I'm looking at device trees right now, and I can understand
> that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
> I don't understand at all why it is &pci1 on the mpc85??cds.dts.

Maybe the ISA IRQ on that board is routed to a PCI IRQ# line in which
case it will use the ISA bridge pci device to lookup in the PCI
interrupt map... it's a bit weird but as long as it resolves using the
standard parser, it's perfectly fine.

> But the definition of the ISA bridge in these files is very strange to 
> start with: I've never seen an ISA bridge with only an interrupt controller 
> on it, no interrupts are connected to it and its reg property is
> almost certainly wrong. Maybe it is an example of things that should
> not be done.

Heh, dunno, I haven't looked that closely at what they did... I would
have expected legacy devices to have their IRQ pointing to the 8259
indeed.

Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

> But feel free to disagree, it's not of terrible importance. The board
> code is the one to "find" the PICs, including the cascaded 8259, the
> acutal 8259 driver takes whatever node is given to it, so as long as
> your .dts matches your board, it doesn't matter that much.

Here at least we do agree -- with the one added nit that
when using "chrp,iic" you're _forcing_ it to be platform
code that handles this device.  But like I said before,
not all that important.

>> This said, I'm looking at device trees right now, and I can understand
>> that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
>> I don't understand at all why it is &pci1 on the mpc85??cds.dts.
>
> Maybe the ISA IRQ on that board is routed to a PCI IRQ# line in which
> case it will use the ISA bridge pci device to lookup in the PCI
> interrupt map... it's a bit weird but as long as it resolves using the
> standard parser, it's perfectly fine.

I don't think it resolves using the _standard_ parser,
it needs a few of the workarounds.  But the tree there
is completely broken anyway, fixing it is highly
recommended ;-)


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Jon Loeliger]

So, like, the other day Gabriel Paubert mumbled:
> 
> This said, I'm looking at device trees right now, and I can understand
> that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
> I don't understand at all why it is &pci1 on the mpc85??cds.dts.
> 
> But the definition of the ISA bridge in these files is very strange to 
> start with: I've never seen an ISA bridge with only an interrupt controller 
> on it, no interrupts are connected to it and its reg property is
> almost certainly wrong. Maybe it is an example of things that should
> not be done.

I think this is an example of Early Days Misunderstanding Creep.
It was likely one of the very first attempts to write the file for
any FSL embedded part, and as such, it was probably not so good.

Likely, it could use some cleanup.  Patches and suggestions welcome.

jdl

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

> I think this is an example of Early Days Misunderstanding Creep.

Is that the official name?  :-)

> It was likely one of the very first attempts to write the file for
> any FSL embedded part, and as such, it was probably not so good.

But it also probably worked just fine, given all the
big fat workarounds in the Linux tree parsing code,
which are applied on *all* boards -- this will have
to change sooner or later, just like you don't run
all PCI quirks on all boards: it simply doesn't scale
and the side effects make everything impossible to
understand.  In almost all cases, simply fixing the
device tree (Linux' runtime copy, anyway) in some
pre-processing pass is conceptually a much simpler
solution as well.

> Likely, it could use some cleanup.  Patches and suggestions welcome.

I rather just wait until things break and then
deprecate (and later remove) support for those
boards.  Unless the respective maintainers for
those boards fix things up, all is fine then
of course ;-)

Slightly more seriously -- since there are so
many DTS files, and their number grows real fast;
and since they are non-modular, and mostly copy-
and-paste; most fixes have to be made to lots
of separate DTS files.  And that is something
no one can do, since no one can *test* those fixes,
since you need the hardware to do that.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Andy Fleming]

On Jun 3, 2007, at 03:33, Gabriel Paubert wrote:

> On Sun, Jun 03, 2007 at 09:41:33AM +0200, Segher Boessenkool wrote:
>>>> Oh what the hell, I'm too curious...  "pnpPNP,0" it is.
>>>>
>>>>> I believe that "8259" should appear somewhere because of the
>>>>> "8259-interrupt-acknowledge" property (defined in CHRP bindings)
>>>>> which you can have on the parent bridge to speed up interrupt
>>>>> vector acquisition.
>>>>
>>>> You're not CHRP so you have nothing to do with the CHRP
>>>> bindings...
>>>
>>> Still, you are close to CHRP
>>
>> Not at all, the rest of the device interrupt subsystem
>> is very different, too.
>
> Given how well you agree, I understand how other people
> might get a bit confused ;-)
>
> This said, I'm looking at device trees right now, and I can understand
> that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
> I don't understand at all why it is &pci1 on the mpc85??cds.dts.
>
> But the definition of the ISA bridge in these files is very strange to
> start with: I've never seen an ISA bridge with only an interrupt  
> controller
> on it, no interrupts are connected to it and its reg property is
> almost certainly wrong. Maybe it is an example of things that should
> not be done.


I take full responsibility for that.  The CDS is a strange beast, so  
I did some strange things in the DTS.

The i8259 is on a subsidiary bus, and so it's one bus #1.  I chose  
not to create a proper node around it.  Not for any good reason.  The  
reason its interrupt parent is pci1 is because the i8259 emits its  
irq though PCI.  This is because the 8555 (and other CDS chips) are  
actually on a PCI card which is connected through PCI 1, while the  
i8259 is behind a bridge on the motherboard into which the CDS  
Carrier Card is plugged.

I always find my grammar stretched to the breaking point when  
describing the PCI topology of the CDS!

The 8641's topology is much simpler, and the i8259 sends its  
interrupts over an external interrupt pin which goes through the mpic/ 
openpic.


Andy

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Randy Vinson]

Andy Fleming wrote:
> On Jun 3, 2007, at 03:33, Gabriel Paubert wrote:
> 
[snippage]
>>
>> This said, I'm looking at device trees right now, and I can understand
>> that interrupt-parent of the 8259 is &mpic in mpc8641_hpcn.dts, but
>> I don't understand at all why it is &pci1 on the mpc85??cds.dts.
>>
>> But the definition of the ISA bridge in these files is very strange to
>> start with: I've never seen an ISA bridge with only an interrupt  
>> controller
>> on it, no interrupts are connected to it and its reg property is
>> almost certainly wrong. Maybe it is an example of things that should
>> not be done.
> 
> 
> I take full responsibility for that.  The CDS is a strange beast, so  
> I did some strange things in the DTS.
> 
> The i8259 is on a subsidiary bus, and so it's one bus #1.
Actually, that's not strictly true. If a card containing a P2P bridge is
installed into PCI slot 4, the i8259 will end up on a bus that is not
numbered bus 1. I've often wondered how something like that is handled
in the DTS.

Randy V.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Andy Fleming]

>>
>> I take full responsibility for that.  The CDS is a strange beast, so
>> I did some strange things in the DTS.
>>
>> The i8259 is on a subsidiary bus, and so it's one bus #1.
> Actually, that's not strictly true. If a card containing a P2P  
> bridge is
> installed into PCI slot 4, the i8259 will end up on a bus that is not
> numbered bus 1. I've often wondered how something like that is handled
> in the DTS.

*Waves hands distractingly*

Pay no attention to the man behind the dts!

Seriously, though, it's quite fragile right now.  We probably need to  
change u-boot to correctly number those things.  And we should  
probably change the dts files for each board to have any soldered-on  
pci devices.

Right now, though, we'd settle for being able to *use* those devices  
under a default setting.

Andy

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Randy Vinson]

Andy Fleming wrote:
>>>
>>> I take full responsibility for that.  The CDS is a strange beast, so
>>> I did some strange things in the DTS.
>>>
>>> The i8259 is on a subsidiary bus, and so it's one bus #1.
>> Actually, that's not strictly true. If a card containing a P2P bridge is
>> installed into PCI slot 4, the i8259 will end up on a bus that is not
>> numbered bus 1. I've often wondered how something like that is handled
>> in the DTS.
> 
> *Waves hands distractingly*
> 
> Pay no attention to the man behind the dts!
> 
> Seriously, though, it's quite fragile right now.  We probably need to
> change u-boot to correctly number those things.  And we should probably
> change the dts files for each board to have any soldered-on pci devices.
> 
> Right now, though, we'd settle for being able to *use* those devices
> under a default setting.
I completely agree an all points. As I get more time to work on
DTS-based hardware, I hope to contribute to the solution. Right now, I'm
trying to handle the i8259 cascade interrupt properly in the context of
Real-Time's threaded interrupts.

Randy V.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Mon, 2007-06-04 at 15:31 -0700, Randy Vinson wrote:
> Actually, that's not strictly true. If a card containing a P2P bridge
> is
> installed into PCI slot 4, the i8259 will end up on a bus that is not
> numbered bus 1. I've often wondered how something like that is handled
> in the DTS.

The bus number shouldn't matter as long as low IO cycles are properly
forwarded to it.

Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Andy Fleming]

On Jun 6, 2007, at 02:09, Benjamin Herrenschmidt wrote:

> On Mon, 2007-06-04 at 15:31 -0700, Randy Vinson wrote:
>> Actually, that's not strictly true. If a card containing a P2P bridge
>> is
>> installed into PCI slot 4, the i8259 will end up on a bus that is not
>> numbered bus 1. I've often wondered how something like that is  
>> handled
>> in the DTS.
>
> The bus number shouldn't matter as long as low IO cycles are properly
> forwarded to it.

Right.  Unless there's an interrupt mapping, which has to include the  
bus number in the filter.  When that happens, parsing the interrupt  
map for the i8259's mapping fails because its address doesn't match.

Andy

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> The bus number shouldn't matter as long as low IO cycles are properly
>> forwarded to it.
>
> Right.  Unless there's an interrupt mapping, which has to include the
> bus number in the filter.

No it doesn't; the PCI device # of the thing that gets
its interrupt mapped is enough.  PCI bus # is not a
hardware property (except for bus #0) so it isn't a
structural thing in the device tree at all.

> When that happens, parsing the interrupt
> map for the i8259's mapping fails because its address doesn't match.

That's because you're trying to map interrupts from
two separate interrupt domains in your interrupt-map:
the PCI bus of that node, and some child PCI bus
somewhere deeper down.  This isn't designed to work,
and hey, it doesn't ;-)


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Thu, 2007-06-07 at 11:21 -0500, Andy Fleming wrote:
> > The bus number shouldn't matter as long as low IO cycles are
> properly
> > forwarded to it.
> 
> Right.  Unless there's an interrupt mapping, which has to include
> the  
> bus number in the filter.  When that happens, parsing the interrupt  
> map for the i8259's mapping fails because its address doesn't match. 

Hrm... does the interrupt-map include the bus number for you ? They
generally don't ...

Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> Right.  Unless there's an interrupt mapping, which has to include
>> the
>> bus number in the filter.  When that happens, parsing the interrupt
>> map for the i8259's mapping fails because its address doesn't match.
>
> Hrm... does the interrupt-map include the bus number for you ? They
> generally don't ...

Any interrupt mapping should serve one interrupt domain
only.  For a PCI bus, that bus is that domain, so all
bus#s in the interrupt-map should be identical.

Now you can cheat and have one interrupt-map handle
mappings for several PCI busses (in the same PCI domain).
Whenever this works at all, it is not by design, and
results in a huge mess and various (little) problems.

Just don't :-)


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Fri, 2007-06-08 at 10:29 +0200, Segher Boessenkool wrote:
> > Hrm... does the interrupt-map include the bus number for you ? They
> > generally don't ...
> 
> Any interrupt mapping should serve one interrupt domain
> only.  For a PCI bus, that bus is that domain, so all
> bus#s in the interrupt-map should be identical.
> 
> Now you can cheat and have one interrupt-map handle
> mappings for several PCI busses (in the same PCI domain).
> Whenever this works at all, it is not by design, and
> results in a huge mess and various (little) problems.

Agreed.

Ben.

RE: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Zang Roy-r61911]

>=20
> >> Yes, I think we all agree -- it should be interrupt-controller@20,
> >> with 20, a0, 4d0 in the "reg" property.
> >
> > In the current tree it is called "i8259" in mpc8641_hpcn.dts.
>=20
> "interrupt-controller" is the preferred name for all
> interrupt controller nodes.  The "compatible" property
> can easily distinguish between different types.  Not
> that it matters all that much -- just remember that
> the "name" is used for device matching before "compatible"
> is (_should_ be, Linux ignores this part of the standard
> right now in most cases), so don't put junk in there!
>=20
> > The other
> > ones which have a 8259 are not usable (mpc8555 for example). I don't
> > even understand their reg property (19000 0 0 0 1),
>=20
> Very weird indeed.  What bustype is this?  (I looked it up,
> it's PCI, and I can't make heads or tails of it either --
> it says it sits on bus #1, although its parent isn't bus #1;
> and the final "1" should be "0".  Many more weird things
> in that node, too).
This 8259 controller in a VIA82C686 chip is behind a tsi310 bridge  on
pci1.
Bus #1 should correct.
It is not enabled in 8555CDS board.
Roy

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Sat, 2007-06-02 at 11:01 +0200, Segher Boessenkool wrote:
> 
> I'm not sure what "compatible" should be for this node, someone
> else can dig that up :-)

chrp,iic

Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

> By far the most important registers are the ones at 0x20 since
> you access them at every interrupt. The registers at 0x4d0
> are typically set by firmware and never touched later, there
> is not a single access to them in sysdev/i8259.c.
> 
> I have vague memories of a node named i8259@20 one a board 
> who had OF in 1997 (a Motorola MVME but they switched to 
> PPCBUG just after :-().

Here's the entry for a 8259 in an old pSeries device-tree:

(Note the use of the recommended naming practice where the name is
actually the device-class)

/proc/device-tree/pci@fef00000/isa@b/interrupt-controller@i20:
name             "interrupt-controller"
linux,phandle    00ceed58 (13561176)
interrupt-parent 00c73068 (13054056)
interrupt-controller

#interrupt-cells 00000002
interrupts       00000000 00000002
reserved-interrupts
                 00000002 00000003
reg              00000001 00000020 00000002 00000001
                 000000a0 00000002 00000001 000004d0
                 00000002
compatible       "chrp,iic"
built-in
model            "WINB,W83C553"
device_type      "interrupt-controller"
ibm,loc-code     "P2"

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Geert Uytterhoeven]

On Sat, 2 Jun 2007, Gabriel Paubert wrote:
> I have vague memories of a node named i8259@20 one a board 
> who had OF in 1997 (a Motorola MVME but they switched to 
> PPCBUG just after :-().

On my LongTrail (RIP), I used to have /pci/isa@1/interrupt-controller@i20
(http://users.telenet.be/geertu/Linux/PPC/pci/isaAT1/interrupt-controllerATi20/)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Jon Loeliger]

On Sat, 2007-06-02 at 03:22, Segher Boessenkool wrote:
> >> "reg" included in "ranges"?  Something is wrong here.
> >
> > I think it's correct for soc nodes.
> 
> It is not.  "ranges" is the address space translation
> between the parent and child busses; "reg" is the stuff
> that's on the bridge device itself.
> 
> > At least, it appears that all of
> > the dts files with soc nodes do similar things (including this one even
> > without this patch).
> 
> Yes, but that doesn't make it right.

But removing it does break things.  I'm going to leave 
it as-is for now, and we can wrestle over "fixing" it
in a follow up patch.

Thanks,
jdl

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>>>> "reg" included in "ranges"?  Something is wrong here.
>>>
>>> I think it's correct for soc nodes.
>>
>> It is not.  "ranges" is the address space translation
>> between the parent and child busses; "reg" is the stuff
>> that's on the bridge device itself.
>>
>>> At least, it appears that all of
>>> the dts files with soc nodes do similar things (including this one 
>>> even
>>> without this patch).
>>
>> Yes, but that doesn't make it right.
>
> But removing it does break things.

What breaks?  This sounds like a Linux bug, in
your port perhaps, or in some OF parsing code.

> I'm going to leave
> it as-is for now, and we can wrestle over "fixing" it
> in a follow up patch.

Fine with me -- please figure out what exactly it
is that is breaking though.

Thanks,


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Fri, 2007-06-01 at 20:58 +0200, Segher Boessenkool wrote:
> > +		ranges = <00000000 f8000000 00100000
> > +			  80000000 80000000 20000000
> > +			  e2000000 e2000000 00100000
> > +			  a0000000 a0000000 20000000
> > +			  e3000000 e3000000 00100000>;
> >  		reg = <f8000000 00100000>;	// CCSRBAR 1M
> 
> 
> "reg" included in "ranges"?  Something is wrong here.

Indeed. To be more precise, "ranges" is what gets forwarded to childs.
"reg" is what gets decoded by the bridge itself...

Cheers,
Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Jon Loeliger]

So, like, the other day Segher Boessenkool mumbled:
> 
> "reg" included in "ranges"?  Something is wrong here.
> 
> You map the same range (4kB legacy I/O @ 0) for both
> bridges here.
> 
> And this address space is included in both of those.
> 
> These need a "reg" property.
> 
> Inclusive again.

> Needs "reg".  And 4d0 isn't the primary address
> I think?

And none of this could have been said earlier when
Wade first posted the patch?

jdl

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Fri, 2007-06-01 at 18:36 -0500, Jon Loeliger wrote:
> 
> And none of this could have been said earlier when
> Wade first posted the patch? 

That's unfair... the traffic on the list is such that he may well have
missed it. I can't review everything myself neither, though I try to
spot things here or there.

Cheers,
Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> And none of this could have been said earlier when
>> Wade first posted the patch?
>
> That's unfair... the traffic on the list is such that he may well have
> missed it. I can't review everything myself neither, though I try to
> spot things here or there.

That, and I often get tired of seeing the same mistakes
over and over again.  When I see a proposed tree that's
just too awful I don't really know where to start.

I also find the DTS source format a bit hard to read,
but maybe that's just me.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Jon Loeliger]

So, like, the other day Segher Boessenkool mumbled:
> 
> That, and I often get tired of seeing the same mistakes
> over and over again.  When I see a proposed tree that's
> just too awful I don't really know where to start.

That is a good point.

I think there are several factors at work here.  First and
foremost, I am not an expert in this area; it is new to me.
I am not afraid to say that I really need the guidance of
an expert here.  Second, perhaps there is lingering legacy
crap in some of the early DTS files that should be systematically
cleaned up.  Many of them were written during a time when we
were really first learning about the whole Device Tree.  While
they may have been there on some, say, Apple boards, it's all
new for the FSL embedded parts.  Cloned mistakes then likely
contributed to the problems and should be fixed.  Finally,
perhaps our documentation on how some of the important fields
should work or be derived is lacking and could be improved.
Suggestions or patches down this line would likely be welcomed.

> I also find the DTS source format a bit hard to read,
> but maybe that's just me.

I, at least, am open to suggestions that might improve it.

Thanks,
jdl

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> That, and I often get tired of seeing the same mistakes
>> over and over again.  When I see a proposed tree that's
>> just too awful I don't really know where to start.
>
> That is a good point.
>
> I think there are several factors at work here.  First and
> foremost, I am not an expert in this area; it is new to me.
> I am not afraid to say that I really need the guidance of
> an expert here.

Feel free to ask for advice on IRC, on the list, or
elsewhere _before_ sending a patch.

> Second, perhaps there is lingering legacy
> crap in some of the early DTS files that should be systematically
> cleaned up.

Yes there is.  Maybe one day I'll get to it, I'm hoping
someone else will fix this though ;-)

> Many of them were written during a time when we
> were really first learning about the whole Device Tree.  While
> they may have been there on some, say, Apple boards, it's all
> new for the FSL embedded parts.

Many apple trees (pun intended) aren't all that great
either.

> Cloned mistakes then likely
> contributed to the problems and should be fixed.  Finally,
> perhaps our documentation on how some of the important fields
> should work or be derived is lacking and could be improved.

The official (and unofficial) Open Firmware docs are quite
good.  Perhaps we need a big huge fat sign in the DTS docs
pointing to them.

> Suggestions or patches down this line would likely be welcomed.
>
>> I also find the DTS source format a bit hard to read,
>> but maybe that's just me.
>
> I, at least, am open to suggestions that might improve it.

My biggest problem when reviewing patches (or whole trees)
in mail is the indenting.  Part of that I could fix myself
by setting my mail reader to eight spaces indent.  The bigger
problem is that trees are just not very readable in flat
text format (you really need to see the parent node together
with every of its child nodes).  There's no real way to fix
this I'm afraid.  In "real" OF, most properties are filled
in programmatibly, and you typically have one node per source
file.  This won't help for DTS files.  Maybe putting more
(good!) comments in DTS files would help though?


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

On Sat, 2007-06-02 at 22:00 +0200, Segher Boessenkool wrote:
> 
> Feel free to ask for advice on IRC, on the list, or
> elsewhere _before_ sending a patch.

Bah, sending a patch is good too, don't be too hard there. I for one
prefer the approach of sending a patch first.

> > Second, perhaps there is lingering legacy
> > crap in some of the early DTS files that should be systematically
> > cleaned up.
> 
> Yes there is.  Maybe one day I'll get to it, I'm hoping
> someone else will fix this though ;-)

We probably need to improve the documentation in fact. Maybe with a step
by step HOWTO create your device-tree with examples for typical things
like PCI etc.. and the rational of why for each of these, we chose to do
XXX instead of YYY...

> The official (and unofficial) Open Firmware docs are quite
> good.  Perhaps we need a big huge fat sign in the DTS docs
> pointing to them.

They aren't -that- good when you come out of the blue and don't know
where to start. I think a tutorial based on building a tree for an
example board step by step would be best.

Ben.

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> Feel free to ask for advice on IRC, on the list, or
>> elsewhere _before_ sending a patch.
>
> Bah, sending a patch is good too, don't be too hard there. I for one
> prefer the approach of sending a patch first.

I'm saying that if you're unsure about stuff, you can ask
about it -- sending a preliminary patch with the question
is one way, sure.

>>> Second, perhaps there is lingering legacy
>>> crap in some of the early DTS files that should be systematically
>>> cleaned up.
>>
>> Yes there is.  Maybe one day I'll get to it, I'm hoping
>> someone else will fix this though ;-)
>
> We probably need to improve the documentation in fact. Maybe with a 
> step
> by step HOWTO create your device-tree with examples for typical things
> like PCI etc.. and the rational of why for each of these, we chose to 
> do
> XXX instead of YYY...

A howto-like doc would help yes.  Rationale might help people
better understand how things work, too, sure.

>> The official (and unofficial) Open Firmware docs are quite
>> good.  Perhaps we need a big huge fat sign in the DTS docs
>> pointing to them.
>
> They aren't -that- good when you come out of the blue and don't know
> where to start.

Well most people don't even know those docs _exist_ it
seems.

> I think a tutorial based on building a tree for an
> example board step by step would be best.

Would be nice yes.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Olof Johansson]

On Sun, Jun 03, 2007 at 09:16:42AM +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2007-06-02 at 22:00 +0200, Segher Boessenkool wrote:
> > The official (and unofficial) Open Firmware docs are quite
> > good.  Perhaps we need a big huge fat sign in the DTS docs
> > pointing to them.
> 
> They aren't -that- good when you come out of the blue and don't know
> where to start. I think a tutorial based on building a tree for an
> example board step by step would be best.

There seems to be two things that causes problems:

1) People basing their new tree on an old example that's broken, and
getting crap for that because it's not correct

2) Trying to add new devices that aren't in any trees already, and
the near infinite discussions that then start.

(1) Could clearly be solved to a fair extent by having a "blessed
golden tree" that people can clone their own board files from, instead
of grabbing a random one and hoping that it's not too broken.

(2) I don't have a good solution for. Better documentation pointers would
definitely be a help, but it's not the whole solution. I'd expect it to
get better and better over time as there'll be less and less unmapped
terrain, and better and better examples to use.


-Olof

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

>> They aren't -that- good when you come out of the blue and don't know
>> where to start. I think a tutorial based on building a tree for an
>> example board step by step would be best.
>
> There seems to be two things that causes problems:

0) The lack of good, clear, exhaustive, and recipe-style
documentation.

> 1) People basing their new tree on an old example that's broken, and
> getting crap for that because it's not correct

Yeah.

> 2) Trying to add new devices that aren't in any trees already, and
> the near infinite discussions that then start.

This is a *good* thing (as long as it's productive) --
it simply is a lot of work to define a good device binding
for non-trivial devices.

> (2) I don't have a good solution for. Better documentation pointers 
> would
> definitely be a help, but it's not the whole solution. I'd expect it to
> get better and better over time as there'll be less and less unmapped
> terrain, and better and better examples to use.

We can only hope :-)


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

> And none of this could have been said earlier when
> Wade first posted the patch?

Could have, sure.  I don't have time to comment on
everything though, some stuff falls through the
cracks.  If you want to change this situation, I'm
sure you can come up with something.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Benjamin Herrenschmidt]

Minor nits (in addition to needing a "reg" property)

> +
> +						i8259: i8259@4d0 {

Name should probably be "interrupt-controller" as well to follow the
recommended naming practice.

> +							clock-frequency = <0>;
> +							interrupt-controller;
> +							device_type = "interrupt-controller";
> +							#address-cells = <0>;
> +							#interrupt-cells = <2>;
> +							built-in;
> +							compatible = "chrp,iic";
> +							big-endian;

big-endian ? On a 8259 ? 

> +							interrupts = <49 2>;
> +							interrupt-parent =
> +								<&mpic>;
> +						};
> +					};
> +				};
>  			};
>  
>  		};

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Gabriel Paubert]

On Sun, Jun 03, 2007 at 09:50:53AM +1000, Benjamin Herrenschmidt wrote:
> Minor nits (in addition to needing a "reg" property)
> 
> > +
> > +						i8259: i8259@4d0 {
> 
> Name should probably be "interrupt-controller" as well to follow the
> recommended naming practice.
> 
> > +							clock-frequency = <0>;
> > +							interrupt-controller;
> > +							device_type = "interrupt-controller";
> > +							#address-cells = <0>;
> > +							#interrupt-cells = <2>;
> > +							built-in;
> > +							compatible = "chrp,iic";
> > +							big-endian;
> 
> big-endian ? On a 8259 ? 

Yes, I spotted that too. Funny on a device which is only ever 
accessed through inb/outb! 

Maybe the 0x4d0/4d1 ports can be accessed with 16 bit 
instructions, I don't know. What I know for sure is that
accessing port 0x20 with an inl/outl on some boards
locks immediately the system up (perhaps through infinite
retries).

	Gabriel

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Segher Boessenkool]

> Maybe the 0x4d0/4d1 ports can be accessed with 16 bit
> instructions, I don't know. What I know for sure is that
> accessing port 0x20 with an inl/outl on some boards
> locks immediately the system up (perhaps through infinite
> retries).

And inw/outw?  There are only two bytes of registers
there after all.


Segher

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Gabriel Paubert]

On Sun, Jun 03, 2007 at 09:42:55AM +0200, Segher Boessenkool wrote:
> >Maybe the 0x4d0/4d1 ports can be accessed with 16 bit
> >instructions, I don't know. What I know for sure is that
> >accessing port 0x20 with an inl/outl on some boards
> >locks immediately the system up (perhaps through infinite
> >retries).
> 
> And inw/outw?  There are only two bytes of registers
> there after all.

It does not lock up on the boards I've tested, but the results 
are undefined (and perhaps implementation dependent). The problem
being that most accesses to the first register (command) change 
the definition of which internal register is accessed with the 
second byte.

	Gabriel

Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.

[Jon Loeliger]

On Sat, 2007-06-02 at 18:50, Benjamin Herrenschmidt wrote:
> Minor nits (in addition to needing a "reg" property)
> 
> > +
> > +						i8259: i8259@4d0 {
> 
> Name should probably be "interrupt-controller" as well to follow the
> recommended naming practice.
> 
> > +							clock-frequency = <0>;
> > +							interrupt-controller;
> > +							device_type = "interrupt-controller";
> > +							#address-cells = <0>;
> > +							#interrupt-cells = <2>;
> > +							built-in;
> > +							compatible = "chrp,iic";
> > +							big-endian;
> 
> big-endian ? On a 8259 ? 
> 
> > +							interrupts = <49 2>;
> > +							interrupt-parent =
> > +								<&mpic>;
> > +						};
> > +					};
> > +				};
> >  			};
> >  
> >  		};
> 

Since the interrupt controller issues are a bit unrelated
to the PCI-Express enablement patches here, I'll submit a
follow-up patch set to clean them up.

Thanks,
jdl