[PATCH 0/5] MPC8641 HPCN fixes

[PATCH 0/5] MPC8641 HPCN fixes

[Wade Farnsworth]

Hello all,

These patches fix a few of the peripherals on the MPC8641 HPCN.

Any comments would be appreciated.

Thanks,

--Wade Farnsworth

[PATCH 1/5] Add MTD support to mpc8641_hpcn.dts

[Wade Farnsworth]

Signed-off-by: Wade Farnsworth <wfarnsworth@mvista.com>

---
 arch/powerpc/boot/dts/mpc8641_hpcn.dts |   13 +++++++++++++
 1 file changed, 13 insertions(+)

Index: linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
===================================================================
--- linux-2.6-8641.orig/arch/powerpc/boot/dts/mpc8641_hpcn.dts
+++ linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
@@ -335,4 +335,17 @@
 			big-endian;
 		};
 	};
+
+	flash@ff800000 {
+		device_type = "rom";
+		compatible = "direct-mapped";
+		probe-type = "CFI";
+		reg = <ff800000 00800000>;
+		bank-width = <2>;
+		partitions = <00000000 00300000
+			      00300000 00100000
+			      00400000 00300000
+			      00700000 00100000>;
+		partition-names = "kernel", "firmware b", "fs", "firmware a";
+	};
 };

[PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Wade Farnsworth]

This adds device nodes for the ISA devices on the MPC8641 HPCN to the
dts.  Additionally, it moves the node for the i8259 to the isa bus,
since the i8259 is actually on that bus.


Signed-off-by: Wade Farnsworth <wfarnsworth@mvista.com>

---

 arch/powerpc/boot/dts/mpc8641_hpcn.dts |   48 ++++++++++++++++++-----
 1 file changed, 38 insertions(+), 10 deletions(-)

Index: linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
===================================================================
--- linux-2.6-8641.orig/arch/powerpc/boot/dts/mpc8641_hpcn.dts
+++ linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
@@ -285,17 +285,45 @@
 				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>;
+
+			isa@f0 {
+				device_type = "isa";
 				#interrupt-cells = <2>;
-				built-in;
-				compatible = "chrp,iic";
-                	        big-endian;
-				interrupts = <49 2>;
-				interrupt-parent = <&mpic>;
+				#size-cells = <1>;
+				#address-cells = <2>;
+				reg = <f000 0 0 0 0>;
+				ranges = <1 0 01000000 0 0 00001000>;
+				interrupt-parent = <4d0>;
+
+				8042@60 {
+					device_type = "8042";
+					reg = <1 60 1 1 64 1>;
+					interrupts = <1 3 c 3>;
+					interrupt-parent = <4d0>;
+				};
+
+				rtc@70 {
+					device_type = "rtc";
+					reg = <1 70 2>;
+				};
+
+				gpio@400 {
+					device_type = "gpio";
+					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>;
+				};
 			};
 
 		};

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Jon Loeliger]

On Tue, 2007-05-15 at 11:27, Wade Farnsworth wrote:
> This adds device nodes for the ISA devices on the MPC8641 HPCN to the
> dts.  Additionally, it moves the node for the i8259 to the isa bus,
> since the i8259 is actually on that bus.

This is good.  Couple of quick comments below.


> Index: linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> ===================================================================
> --- linux-2.6-8641.orig/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> +++ linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> @@ -285,17 +285,45 @@


> +			isa@f0 {
> +				device_type = "isa";
>  				#interrupt-cells = <2>;

Yea, thanks.  BTW, will this be sufficient to now identify the
primary controller and not doink with the legacy IO ports now?
Or is that being handled differently somehow?

> +				#size-cells = <1>;
> +				#address-cells = <2>;
> +				reg = <f000 0 0 0 0>;
> +				ranges = <1 0 01000000 0 0 00001000>;
> +				interrupt-parent = <4d0>;

That 4d0 should likely be &i8259.

> +				8042@60 {
> +					device_type = "8042";
> +					reg = <1 60 1 1 64 1>;
> +					interrupts = <1 3 c 3>;
> +					interrupt-parent = <4d0>;
> +				};

Ditto.

> +				rtc@70 {
> +					device_type = "rtc";
> +					reg = <1 70 2>;
> +				};

Is "rtc" a standard OF device, or are we making up a
new thing that should be documented in booting-with-OF.txt ?
And is the plan to somehow use this entry in the future?

Thanks,
jdl

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Wade Farnsworth]

On Wed, 2007-05-16 at 11:37 -0500, Jon Loeliger wrote:
> On Tue, 2007-05-15 at 11:27, Wade Farnsworth wrote:

<snip>

> > +			isa@f0 {
> > +				device_type = "isa";
> >  				#interrupt-cells = <2>;
> 
> Yea, thanks.  BTW, will this be sufficient to now identify the
> primary controller and not doink with the legacy IO ports now?
> Or is that being handled differently somehow?

I'm not sure what you're referring to here.  Can you clarify this a
little?

> 
> > +				#size-cells = <1>;
> > +				#address-cells = <2>;
> > +				reg = <f000 0 0 0 0>;
> > +				ranges = <1 0 01000000 0 0 00001000>;
> > +				interrupt-parent = <4d0>;
> 
> That 4d0 should likely be &i8259.
> 
> > +				8042@60 {
> > +					device_type = "8042";
> > +					reg = <1 60 1 1 64 1>;
> > +					interrupts = <1 3 c 3>;
> > +					interrupt-parent = <4d0>;
> > +				};
> 
> Ditto.

Yes, of course.  Thanks.

> 
> > +				rtc@70 {
> > +					device_type = "rtc";
> > +					reg = <1 70 2>;
> > +				};
> 
> Is "rtc" a standard OF device, or are we making up a
> new thing that should be documented in booting-with-OF.txt ?
> And is the plan to somehow use this entry in the future?

It is my understanding that "rtc" is a standard OF device.  Can anybody
confirm this?

This could be used in the platform-specific RTC code to get the ports
from the device tree.  Currently, however, I simply use RTC_PORT(x) from
asm/mc146818rtc.h.  Is it preferable to use the device tree here?

--Wade

> 
> Thanks,
> jdl
> 
> 

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Segher Boessenkool]

>> Is "rtc" a standard OF device, or are we making up a
>> new thing that should be documented in booting-with-OF.txt ?
>> And is the plan to somehow use this entry in the future?
>
> It is my understanding that "rtc" is a standard OF device.  Can anybody
> confirm this?

It is.

> This could be used in the platform-specific RTC code to get the ports
> from the device tree.  Currently, however, I simply use RTC_PORT(x) 
> from
> asm/mc146818rtc.h.  Is it preferable to use the device tree here?

If you instantiate the RTC device from platform code,
and the device sits at the same address on all boards
you support, the only good thing you get from using
the device tree is that you don't have to hardcode a
number in the Linux code.  Using the device tree however
the RTC device could be detected and instantiated from
more generic code, a much better situation.


Segher

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Segher Boessenkool]

>> +			isa@f0 {

isa@1e

>> +				8042@60 {
>> +					device_type = "8042";

Drop the device_type.  A number as a name isn't
all that great, either.

>> +				rtc@70 {
>> +					device_type = "rtc";
>> +					reg = <1 70 2>;
>> +				};
>
> Is "rtc" a standard OF device,

Yes, it is defined in the "Device Support Extensions"
recommended practice.

> And is the plan to somehow use this entry in the future?

The platform code can use it to find the device.  If it
would have a proper "compatible" property, the generic
code could do that work, instead.


Segher

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:

>>>+				8042@60 {
>>>+					device_type = "8042";

> Drop the device_type.  A number as a name isn't
> all that great, either.

    Perhaps it should be called "keyboard" (and device_type should also be set 
to it) since 8042 is in fact (mostly) a keyboard controller... although it 
also "drives" PS/2 mouse...

>>>+				rtc@70 {
>>>+					device_type = "rtc";
>>>+					reg = <1 70 2>;
>>>+				};

>>Is "rtc" a standard OF device,

> Yes, it is defined in the "Device Support Extensions"
> recommended practice.

    And it also defines "keyboard".

WBR, Sergei

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Wade Farnsworth]

On Wed, 2007-05-16 at 21:55 +0200, Segher Boessenkool wrote:
> >> +			isa@f0 {
> 
> isa@1e
> 
> >> +				8042@60 {
> >> +					device_type = "8042";
> 
> Drop the device_type.  A number as a name isn't
> all that great, either.

Currently in order for the i8042 devices to be initialized,
check_legacy_ioport() must find a node with device_type "8042".

> 
> >> +				rtc@70 {
> >> +					device_type = "rtc";
> >> +					reg = <1 70 2>;
> >> +				};
> >
> > Is "rtc" a standard OF device,
> 
> Yes, it is defined in the "Device Support Extensions"
> recommended practice.
> 
> > And is the plan to somehow use this entry in the future?
> 
> The platform code can use it to find the device.  If it
> would have a proper "compatible" property, the generic
> code could do that work, instead.

Is there a recommended "compatible" property for a standard PC-like CMOS
RTC?

--Wade

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Segher Boessenkool]

>>>> +			isa@f0 {
>>
>> isa@1e

You didn't reply to this one, but you *really* need
to fix it.

>>>> +				8042@60 {
>>>> +					device_type = "8042";
>>
>> Drop the device_type.  A number as a name isn't
>> all that great, either.
>
> Currently in order for the i8042 devices to be initialized,
> check_legacy_ioport() must find a node with device_type "8042".

So fix that :-)

> Is there a recommended "compatible" property for a standard PC-like 
> CMOS
> RTC?

None in the bindings that I know of.  It typically would
be the model name/number of the oldest chip used, or one
that is very commonly used.


Segher

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Segher Boessenkool]

>>>>> +			isa@f0 {
>>>
>>> isa@1e
>
> You didn't reply to this one, but you *really* need
> to fix it.

Oh you did reply, heh.

>> Is there a recommended "compatible" property for a standard PC-like
>> CMOS
>> RTC?
>
> None in the bindings that I know of.  It typically would
> be the model name/number of the oldest chip used, or one
> that is very commonly used.

"pnpPNP,b00"


Segher

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Wade Farnsworth]

On Thu, 2007-05-17 at 01:47 +0200, Segher Boessenkool wrote:
> >> Is there a recommended "compatible" property for a standard PC-like
> >> CMOS
> >> RTC?
> >
> > None in the bindings that I know of.  It typically would
> > be the model name/number of the oldest chip used, or one
> > that is very commonly used.
> 
> "pnpPNP,b00"
> 
> 
> Segher
> 

Thanks.  I'll look into making the RTC set up code generic.

--Wade

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Wade Farnsworth]

On Thu, 2007-05-17 at 01:40 +0200, Segher Boessenkool wrote:
> >>>> +				8042@60 {
> >>>> +					device_type = "8042";
> >>
> >> Drop the device_type.  A number as a name isn't
> >> all that great, either.
> >
> > Currently in order for the i8042 devices to be initialized,
> > check_legacy_ioport() must find a node with device_type "8042".
> 
> So fix that :-)

No problem for this platform.  Are there any device trees for other
platforms that use this?

--Wade

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Jon Loeliger]

On Wed, 2007-05-16 at 18:40, Segher Boessenkool wrote:

> > Is there a recommended "compatible" property for a standard PC-like 
> > CMOS
> > RTC?
> 
> None in the bindings that I know of.  It typically would
> be the model name/number of the oldest chip used, or one
> that is very commonly used.

And I haven't actually gotten it to work yet either...
Am I missing something?  Like a CONFIG_ option beyond
the RTC and CMOS variant that is necessary?

Dumbly,
jdl

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Wade Farnsworth]

On Fri, 2007-05-18 at 11:44 -0500, Jon Loeliger wrote:
> 
> And I haven't actually gotten it to work yet either...
> Am I missing something?  Like a CONFIG_ option beyond
> the RTC and CMOS variant that is necessary?
> 
> Dumbly,
> jdl
> 
> 

I wonder if you're running into the locked RTC problem on the m1575.
The version of u-boot that I'm using (recent pull from
opensource.freescale.com) has a workaround for that.  Can you verify if
this is the problem?

For the benefit of those who aren't aware of the problem, the RTC on the
m1575 is locked on boot until PCI memory space is read.

Maybe we should put the workaround in the kernel instead.  Any opinions?

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Jon Loeliger]

On Mon, 2007-05-21 at 11:04, Wade Farnsworth wrote:

> I wonder if you're running into the locked RTC problem on the m1575.
> The version of u-boot that I'm using (recent pull from
> opensource.freescale.com)

Which repository?  I ask because it is highly likely that
I placed it there...

>  has a workaround for that.  Can you verify if
> this is the problem?

Can you point to "the workaround" to which you refer?

> For the benefit of those who aren't aware of the problem, the RTC on the
> m1575 is locked on boot until PCI memory space is read.

Hmmm.  I'm pretty sure this is being done already.

> Maybe we should put the workaround in the kernel instead.  Any opinions?

How are you getting it to work now as it is, then?

jdl

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Wade Farnsworth]

On Mon, 2007-05-21 at 11:10 -0500, Jon Loeliger wrote:
> On Mon, 2007-05-21 at 11:04, Wade Farnsworth wrote:
> 
> > I wonder if you're running into the locked RTC problem on the m1575.
> > The version of u-boot that I'm using (recent pull from
> > opensource.freescale.com)
> 
> Which repository?  I ask because it is highly likely that
> I placed it there...

http://opensource.freescale.com/pub/scm/u-boot-jdl.git

> 
> >  has a workaround for that.  Can you verify if
> > this is the problem?
> 
> Can you point to "the workaround" to which you refer?

commit a59a76edd7c475329c083d5c21f6bca8bb5b8476

Re-enable a PCIE hack access for RTC on 8641HPCN.

> 
> > For the benefit of those who aren't aware of the problem, the RTC on the
> > m1575 is locked on boot until PCI memory space is read.
> 
> Hmmm.  I'm pretty sure this is being done already.

If you're sure your u-boot is doing this, then maybe it is a
configuration problem after all.

> 
> > Maybe we should put the workaround in the kernel instead.  Any opinions?
> 
> How are you getting it to work now as it is, then?

In addition to the u-boot hack, I have the following config options
enabled:

CONFIG_PCI=y
CONFIG_RTC_LIB=y
CONFIG_RTC_CLASS=y
CONFIG_RTC_HCTOSYS=y
CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
CONFIG_RTC_INTF_SYSFS=y
CONFIG_RTC_INTF_PROC=y
CONFIG_RTC_INTF_DEV=y
CONFIG_RTC_DRV_CMOS=y

I believe this is all you should need to enable the RTC.  You should
then see a boot message indicating that the system clock has been set:

rtc_cmos rtc_cmos: setting the system clock to 2007-05-21 21:29:13
(1179782953)

In order to use hwclock to access the RTC you have to make a symlink
from /dev/rtc0 to /dev/rtc.

--Wade

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Wade Farnsworth]

On Wed, 2007-05-16 at 21:55 +0200, Segher Boessenkool wrote:
> >> +			isa@f0 {
> 
> isa@1e
> 

Oh by the way, thanks for pointing this out.  Will fix.

--Wade

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:
>>>+			isa@f0 {

> isa@1e

>>>+				8042@60 {
>>>+					device_type = "8042";

> Drop the device_type.  A number as a name isn't
> all that great, either.

   Yet it's kinda accepted years ago, see:

http://playground.sun.com/1275/proposals/Closed/Accepted/381-it.txt

    Ugh, it's hard to find these documents... All "name", "device_type", and 
"compatible" must all be "8042".

> Segher

WBR, Sergei

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Arnd Bergmann]

On Saturday 07 July 2007, Sergei Shtylyov wrote:
> =A0 =A0Yet it's kinda accepted years ago, see:
>=20
> http://playground.sun.com/1275/proposals/Closed/Accepted/381-it.txt
>=20
> =A0 =A0 Ugh, it's hard to find these documents... All "name", "device_typ=
e", and=20
> "compatible" must all be "8042".
>=20

Right. I've just checked an OF based machine that has 8042 keyboard
controller (the OLPC laptop), to see what's in there.

Here, we have=20

8042@i60 {
	device_type =3D "8042";
	reg =3D <1 60 1 1 64 1>;
	interrupts <1 3 c 3>;
	/* no interrupt-parent, this gets inherited from ISA node */
	model =3D "INT,80c42";
	compatible =3D "ps2-keyboard-controller" "INTC,80c42";
	name =3D "8042"
	#size-cells <0>;
	#address-cells <1>;

	mouse@aux {
		reg =3D <1>;
		compatible =3D "pnpPNP,f03";
		device_type =3D "mouse";
		name =3D "mouse";
	}

	keyboard@ {
		reg =3D <0>;
		language =3D "EN";
		keyboard-type =3D "us";
		device-type =3D "keyboard";
		compatible =3D "pnpPNP,303";
		name =3D "keyboard";
	}
}

My interpretation of the document you cited is that the #size-cells and
#address-cells as well as the child nodes are actually required by the
standard, but it seems that Linux doesn't care.

The model and compatible properties in the 8042 node are not standard
compliant in my machine, so I'm not sure what to best put in there.
It is very clear on name and device-type though.

	Arnd <><

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Segher Boessenkool]

>>>> +				8042@60 {
>>>> +					device_type = "8042";
>
>> Drop the device_type.  A number as a name isn't
>> all that great, either.
>
>   Yet it's kinda accepted years ago, see:
>
> http://playground.sun.com/1275/proposals/Closed/Accepted/381-it.txt

That's not a published recommendation or anything like that.


Segher

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Arnd Bergmann]

On Sunday 08 July 2007, Segher Boessenkool wrote:
> >>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A08042@60 {
> >>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0device_type =3D "8042";
> >
> >> Drop the device_type. =A0A number as a name isn't
> >> all that great, either.
> >
> > =A0 Yet it's kinda accepted years ago, see:
> >
> > http://playground.sun.com/1275/proposals/Closed/Accepted/381-it.txt
>=20
> That's not a published recommendation or anything like that.
>=20

But still, it's what got used in what appears to be all ISA based machines
with such a keyboard controller, both PowerPC and i386 ones.

Are you suggesting that we should declare all existing machines wrong and do
our own thing instead?

	Arnd <><

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Segher Boessenkool]

>>>>>> +                          8042@60 {
>>>>>> +                                  device_type = "8042";
>>>
>>>> Drop the device_type.  A number as a name isn't
>>>> all that great, either.
>>>
>>>   Yet it's kinda accepted years ago, see:
>>>
>>> http://playground.sun.com/1275/proposals/Closed/Accepted/381-it.txt
>>
>> That's not a published recommendation or anything like that.
>
> But still, it's what got used in what appears to be all ISA based  
> machines
> with such a keyboard controller, both PowerPC and i386 ones.

For PowerPC, I only know about CHRP, which has its own binding
for 8042.  For i386 I think you refer to the OLPC?  It does _not_
comply with this #381 (it has a different unit address text
representation, for one thing).  Also, it has "ps2-interrupt-controller"
in its "compatible" property, which is a much nicer thing to match
drivers on for the OS.

> Are you suggesting that we should declare all existing machines  
> wrong and do
> our own thing instead?

You can read in my words whatever you want, but the only thing
I suggested was what I wrote: drop the device_type.  It is
completely useless in a flat device tree whatever binding you
follow here.

And put something more useful than a bare number in the "compatible"
property.  If you feel you really need to you can _also_ put the
number there, but I think you'll find you don't really want to.

And the "name" should be "keyboard-controller" or something like
that, following the generic names recommended practice.

I would argue the 8042 node shouldn't have a device_type at all
btw, since there is no defined effect of "open"ing this node; but
that is a completely separate discussion.


Segher

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:
>>>>> +                8042@60 {
>>>>> +                    device_type = "8042";

>>> Drop the device_type.  A number as a name isn't
>>> all that great, either.

>>   Yet it's kinda accepted years ago, see:

>> http://playground.sun.com/1275/proposals/Closed/Accepted/381-it.txt

> That's not a published recommendation or anything like that.

    There's also this document with a status of "unapproved draft":

http://playground.sun.com/1275/bindings/devices/html/8042.html

    So, all this never got final approval? Somewhat confusing (well, what one 
could expect from OF? :-)

> Segher

WBR, Sergei

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Segher Boessenkool]

>>>   Yet it's kinda accepted years ago, see:
>
>>> http://playground.sun.com/1275/proposals/Closed/Accepted/381-it.txt
>
>> That's not a published recommendation or anything like that.
>
>    There's also this document with a status of "unapproved draft":
>
> http://playground.sun.com/1275/bindings/devices/html/8042.html
>
>    So, all this never got final approval? Somewhat confusing (well,  
> what one could expect from OF? :-)

I think this 8042 thing became part of the CHRP binding.  There
never was a (published) x86 binding btw.  So for our current
purposes (flat device tree) I recommend we just do the minimal
sane thing, i.e., no device_type and some good "compatible" entry.


Segher

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Sergei Shtylyov]

Wade Farnsworth wrote:
> This adds device nodes for the ISA devices on the MPC8641 HPCN to the
> dts.  Additionally, it moves the node for the i8259 to the isa bus,
> since the i8259 is actually on that bus.

> Signed-off-by: Wade Farnsworth <wfarnsworth@mvista.com>

    Sorry for the belated comments... :-<

> Index: linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> ===================================================================
> --- linux-2.6-8641.orig/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> +++ linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> @@ -285,17 +285,45 @@
>  				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>;
> +
> +			isa@f0 {
> +				device_type = "isa";
>  				#interrupt-cells = <2>;
> -				built-in;
> -				compatible = "chrp,iic";
> -                	        big-endian;
> -				interrupts = <49 2>;
> -				interrupt-parent = <&mpic>;
> +				#size-cells = <1>;
> +				#address-cells = <2>;
> +				reg = <f000 0 0 0 0>;
> +				ranges = <1 0 01000000 0 0 00001000>;
> +				interrupt-parent = <4d0>;
> +
> +				8042@60 {
> +					device_type = "8042";
> +					reg = <1 60 1 1 64 1>;

    What does that leading 1 in the address signify?

> +					interrupts = <1 3 c 3>;
> +					interrupt-parent = <4d0>;
> +				};
> +
> +				rtc@70 {
> +					device_type = "rtc";
> +					reg = <1 70 2>;
> +				};
> +
> +				gpio@400 {
> +					device_type = "gpio";
> +					reg = <1 400 80>;
> +				};
> +
> +				i8259: i8259@4d0 {
> +					clock-frequency = <0>;

    I don't think it's at all applicable here.

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

    Really?

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

    The "reg" property is missing here. And "reserved-interrupts" accordig to:

http://playground.sun.com/1275/bindings/devices/html/isa-pic-1_1d.html

    BTW, I've found the offisial 8042 binding is here:

http://playground.sun.com/1275/bindings/devices/html/8042.html

WBR, Sergei

Re: [PATCH 2/5] Add legacy devices to mpc8641_hpcn.dts

[Arnd Bergmann]

T24gU2F0dXJkYXkgMDcgSnVseSAyMDA3LCBTZXJnZWkgU2h0eWx5b3Ygd3JvdGU6Cj4gPiAroKCg
oKCgoKCgoKCgoKCgoKCgoKCgaXNhQGYwIHsKPiA+ICugoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKCgoGRldmljZV90eXBlID0gImlzYSI7Cj4gPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKAjaW50ZXJydXB0LWNlbGxzID0gPDI+Owo+ID4gLaCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKCgYnVpbHQtaW47Cj4gPiAtoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBjb21wYXRpYmxl
ID0gImNocnAsaWljIjsKPiA+IC0goCCgIKAgoCCgIKAgoCCgoKCgoKAgoCCgIKAgoGJpZy1lbmRp
YW47Cj4gPiAtoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBpbnRlcnJ1cHRzID0gPDQ5IDI+
Owo+ID4gLaCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgaW50ZXJydXB0LXBhcmVudCA9IDwm
bXBpYz47Cj4gPiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKAjc2l6ZS1jZWxscyA9IDwx
PjsKPiA+ICugoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoCNhZGRyZXNzLWNlbGxzID0gPDI+
Owo+ID4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgcmVnID0gPGYwMDAgMCAwIDAgMD47
Cj4gPiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKByYW5nZXMgPSA8MSAwIDAxMDAwMDAw
IDAgMCAwMDAwMTAwMD47Cj4gPiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBpbnRlcnJ1
cHQtcGFyZW50ID0gPDRkMD47Cj4gPiArCj4gPiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKA4MDQyQDYwIHsKPiA+ICugoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgZGV2
aWNlX3R5cGUgPSAiODA0MiI7Cj4gPiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKCgoHJlZyA9IDwxIDYwIDEgMSA2NCAxPjsKPiAKPiCgIKAgV2hhdCBkb2VzIHRoYXQgbGVhZGlu
ZyAxIGluIHRoZSBhZGRyZXNzIHNpZ25pZnk/CgpJdCBzaWduaWZpZXMgYW4gSS9PIHBvcnQgYWRk
cmVzcyBpbnN0ZWFkIG9mIGFuIE1NSU8gYWRkcmVzcywgbm90ZSBob3cKPDEgNjA+IGlzIHRyYW5z
Zm9ybWVkIHRvIDwxMDAwMDAwIDAgNjA+IG9uIFBDSSwgd2hpY2ggaXMgdHJhbnNmb3JtZWQKdG8g
bW1pbyBhZGRyZXNzIDxlMjAwMDAwMCA2MD4gaW4gdGhlIGhvc3QgdGhyb3VnaCB0aGUgdmFyaW91
cyByYW5nZXMKcHJvcGVydGllcy4KCglBcm5kIDw+PAo=

Re: [PATCH 3/5] Support for the LPC47M192 SuperIO on the MPC8641 HPCN

[Wade Farnsworth]

This adds support for the LPC47M192 SuperIO on the MPC8641 HPCN.
Specifically this adds support for the I8042 Keyboard/Mouse and the GPIO
on the chip.

Also, the mouse needs to use IRQ 12, which is currently in use by some
PCI devices.  Move those devices to IRQ 11.

Signed-off-by: Wade Farnsworth <wfarnsworth@mvista.com>

---

 arch/powerpc/boot/dts/mpc8641_hpcn.dts     |    4 -
 arch/powerpc/platforms/86xx/mpc86xx_hpcn.c |   43 +++++++++++++++----
 2 files changed, 38 insertions(+), 9 deletions(-)


Index: linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
===================================================================
--- linux-2.6-8641.orig/arch/powerpc/boot/dts/mpc8641_hpcn.dts
+++ linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
@@ -264,7 +264,7 @@
 				/* IDSEL 0x1c */
 				e000 0 0 1 &i8259 9 2
 				e000 0 0 2 &i8259 a 2
-				e000 0 0 3 &i8259 c 2
+				e000 0 0 3 &i8259 b 2
 				e000 0 0 4 &i8259 7 2
 
 				/* IDSEL 0x1d */
@@ -274,7 +274,7 @@
 				e800 0 0 4 &i8259 0 0
 
 				/* IDSEL 0x1e */
-				f000 0 0 1 &i8259 c 2
+				f000 0 0 1 &i8259 b 2
 				f000 0 0 2 &i8259 0 0
 				f000 0 0 3 &i8259 0 0
 				f000 0 0 4 &i8259 0 0
Index: linux-2.6-8641/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
===================================================================
--- linux-2.6-8641.orig/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
+++ linux-2.6-8641/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
@@ -141,7 +141,7 @@ const unsigned char uli1575_irq_route_ta
 	0x1,	/* 9: 0b0001 */
 	0x3,	/* 10: 0b0011 */
 	0x9,	/* 11: 0b1001 */
-	0xb,	/* 12: 0b1011 */
+	0,	/* 12: Reserved */
 	0, 	/* 13: Reserved */
 	0xd,	/* 14, 0b1101 */
 	0xf,	/* 15, 0b1111 */
@@ -211,7 +211,7 @@ static void __devinit quirk_uli1575(stru
 			pirq_map_word |= (uli1575_irq_route_table[i] & 0xf)
 				<< ((irq2pin[i] - PIRQA) * 4);
 
-	/* ULI1575 IRQ mapping conf register default value is 0xb9317542 */
+	/* ULI1575 IRQ mapping conf register default value is 0x09317542 */
 	DBG("Setup ULI1575 IRQ mapping configuration register value = 0x%x\n",
 			pirq_map_word);
 	pci_write_config_dword(dev, 0x48, pirq_map_word);
@@ -266,9 +266,9 @@ static void __devinit quirk_uli1575(stru
 	pci_write_config_byte(dev, 0x44, 0x30 | uli1575_irq_route_table[14]);
 	pci_write_config_byte(dev, 0x75, uli1575_irq_route_table[15]);
 
-	/* Set IRQ14 and IRQ15 to legacy IRQs */
+	/* Set IRQ1, IRQ12, IRQ14 and IRQ15 to legacy IRQs */
 	pci_read_config_word(dev, 0x46, &temp);
-	temp |= 0xc000;
+	temp |= 0xd002;
 	pci_write_config_word(dev, 0x46, temp);
 
 	/* Set i8259 interrupt trigger
@@ -280,12 +280,12 @@ static void __devinit quirk_uli1575(stru
 	 * IRQ 9:  Level
 	 * IRQ 10: Level
 	 * IRQ 11: Level
-	 * IRQ 12: Level
+	 * IRQ 12: Edge
 	 * IRQ 14: Edge
 	 * IRQ 15: Edge
 	 */
-	outb(0xfa, 0x4d0);
-	outb(0x1e, 0x4d1);
+	outb(0xf8, 0x4d0);
+	outb(0x0e, 0x4d1);
 
 #undef ULI1575_SET_DEV_IRQ
 
@@ -293,6 +293,35 @@ static void __devinit quirk_uli1575(stru
 	pci_read_config_byte(dev, 0xb8, &c);
 	c &= 0x7f;
 	pci_write_config_byte(dev, 0xb8, c);
+
+	/* enable superio @ 0x4e and keyboard/mouse address decoding */
+	pci_write_config_byte(dev, 0x63, 0x90);
+
+	/* LPC47M192 Super I/O configuration */
+	outb(0x55, 0x4e);	/* enter superio config mode */
+
+	/* Enable keyboard and mouse */
+	outb(0x07, 0x4e);	/* device selector register */
+	outb(0x07, 0x4f);	/* select keyboard registers (device 7) */
+	outb(0x30, 0x4e);	/* keyboard activation register */
+	outb(0x01, 0x4f);	/* activate keyboard */
+	outb(0x70, 0x4e);	/* keyboard IRQ register */
+	outb(0x01, 0x4f);	/* IRQ1 for keyboard */
+	outb(0x72, 0x4e);	/* mouse IRQ register */
+	outb(0x0c, 0x4f);	/* IRQ12 for mouse */
+
+	/* Enable superio runtime registers for gpio in pci i/o space */
+	outb(0x20, 0x4e);	/* device id register */
+	outb(0x07, 0x4e);	/* device selector register */
+	outb(0x0a, 0x4f);	/* select runtime registers (device A) */
+	outb(0x60, 0x4e);	/* select runtime register address high byte */
+	outb(0x04, 0x4f);	/* runtime register address high byte */
+	outb(0x61, 0x4e);	/* select runtime register address low byte */
+	outb(0x01, 0x4f);	/* runtime register address low byte */
+	outb(0x30, 0x4e);	/* runtime registers activation register */
+	outb(0x01, 0x4f);	/* activate runtime registers */
+
+	outb(0xaa, 0x4e);	/* exit superio config mode */
 }
 
 static void __devinit quirk_uli5288(struct pci_dev *dev)

[PATCH 4/5] Add RTC CMOS support for the MPC8641 HPCN

[Wade Farnsworth]

This enables the new RTC CMOS driver to be used on the MPC8641 HPCN.

Signed-off-by: Wade Farnsworth <wfarnsworth@mvista.com>

---

 arch/powerpc/platforms/86xx/mpc86xx_hpcn.c |   27 +++++++++++++++++++
 1 file changed, 27 insertions(+)


Index: powerpc-8641/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
===================================================================
--- powerpc-8641.orig/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
+++ powerpc-8641/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
@@ -18,6 +18,7 @@
 #include <linux/kdev_t.h>
 #include <linux/delay.h>
 #include <linux/seq_file.h>
+#include <linux/mc146818rtc.h>
 
 #include <asm/system.h>
 #include <asm/time.h>
@@ -368,6 +369,32 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_A
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x5288, quirk_uli5288);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x5229, quirk_uli5229);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AL, 0x5249, early_uli5249);
+
+#ifdef CONFIG_RTC_DRV_CMOS
+struct resource rtc_platform_resource = {
+	.flags		= IORESOURCE_IO,
+	.start		= RTC_PORT(0),
+	.end		= RTC_PORT(1),
+};
+
+static int  __init mpc8641_hpcn_add_rtc(void)
+{
+	struct platform_device *pd;
+
+	pd = platform_device_register_simple("rtc_cmos", -1,
+					     &rtc_platform_resource, 1);
+	if (IS_ERR(pd))
+		return PTR_ERR(pd);
+
+	/* rtc-cmos only supports 24-hr mode */
+	CMOS_WRITE(CMOS_READ(RTC_CONTROL) | RTC_24H, RTC_CONTROL);
+
+	return 0;
+}
+arch_initcall(mpc8641_hpcn_add_rtc);
+
+#endif /* CONFIG_RTC_DRV_CMOS */
+
 #endif /* CONFIG_PCI */
 
 

[PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02 hardware

[Wade Farnsworth]

This fixes PCI support for newer revisions of the MPCN8641 silicon and
the HPCN hardware.

Currently, there is a workaround for a PCI erratum that forces the host
bridge's primary bus to be 0xff.  This workaround is no longer necessary
on newer revisions.

Additionally, the host bridge on the newer rev's behaves like a
transparent bridge, but does not use the correct PCI class, so this adds
a fixup for the class.  This is the only device on bus 0, all other
devices are on subsequent buses.

Furthermore, all of the devices on the southbridge show up on bus 2,
behind an additional PCI bridge.  In order to use the correct IRQ's for
the devices, we need to create additional device nodes for the PCI
bridges, and reproduce the interrupt map on the correct bus.

Since the southbridge devices are now bus 2, we also need to fix the bus
range for PCIe 2 to begin at bus 3.

Signed-off-by: Wade Farnsworth <wfarnsworth@mvista.com>

---

 arch/powerpc/boot/dts/mpc8641_hpcn.dts     |  121 ++++++++++++++++++-
 arch/powerpc/platforms/86xx/mpc86xx_hpcn.c |    8 -
 arch/powerpc/platforms/86xx/pci.c          |   27 +++-
 3 files changed, 149 insertions(+), 7 deletions(-)On Tue, 2007-05-15 at
09:17 -0700, Wade Farnsworth wrote:


Index: linux-2.6-8641/arch/powerpc/platforms/86xx/pci.c
===================================================================
--- linux-2.6-8641.orig/arch/powerpc/platforms/86xx/pci.c
+++ linux-2.6-8641/arch/powerpc/platforms/86xx/pci.c
@@ -134,16 +134,24 @@ mpc86xx_setup_pcie(struct pci_controller
 
 	early_write_config_byte(hose, 0, 0, PCI_LATENCY_TIMER, 0x80);
 
-	/* PCIE Bus, Fix the MPC8641D host bridge's location to bus 0xFF. */
+	/*
+	 * For Rev 1.0, fix the bus to 0xff.  Otherwise use the default.
+	 */
 	early_read_config_dword(hose, 0, 0, PCI_PRIMARY_BUS, &temps);
-	temps = (temps & 0xff000000) | (0xff) | (0x0 << 8) | (0xfe << 16);
+	if ((mfspr(SPRN_SVR) & 0xf0) == 0x10)
+		temps = (temps & 0xff000000) | 0xff | (0xfe << 16);
+	else
+		temps = (temps & 0xff000000) | hose->first_busno |
+			((hose->first_busno + 1) << 8) |
+			(hose->last_busno << 16);
 	early_write_config_dword(hose, 0, 0, PCI_PRIMARY_BUS, temps);
 }
 
 int mpc86xx_exclude_device(u_char bus, u_char devfn)
 {
-	if (bus == 0 && PCI_SLOT(devfn) == 0)
-		return PCIBIOS_DEVICE_NOT_FOUND;
+	if ((mfspr(SPRN_SVR) & 0xf0) == 0x10)
+		if (bus == 0 && PCI_SLOT(devfn) == 0)
+			return PCIBIOS_DEVICE_NOT_FOUND;
 
 	return PCIBIOS_SUCCESSFUL;
 }
@@ -162,6 +170,11 @@ int __init add_bridge(struct device_node
 	/* Fetch host bridge registers address */
 	has_address = (of_address_to_resource(dev, 0, &rsrc) == 0);
 
+	/* For Rev 1 silicon, only the first PCI host is supported. */
+	if (((mfspr(SPRN_SVR) & 0xf0) == 0x10) &&
+	    ((rsrc.start & 0xfffff) == 0x9000))
+		return 0;
+
 	/* Get bus range if any */
 	bus_range = of_get_property(dev, "bus-range", &len);
 	if (bus_range == NULL || len < 2 * sizeof(int))
@@ -203,3 +216,9 @@ int __init add_bridge(struct device_node
 
 	return 0;
 }
+
+static void __devinit early_mpc86xx_pcie(struct pci_dev *dev)
+{
+	   dev->class = PCI_CLASS_BRIDGE_PCI << 8 | 0x1;
+}
+DECLARE_PCI_FIXUP_EARLY(0x1957, 0x7011, early_mpc86xx_pcie);
Index: linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
===================================================================
--- linux-2.6-8641.orig/arch/powerpc/boot/dts/mpc8641_hpcn.dts
+++ linux-2.6-8641/arch/powerpc/boot/dts/mpc8641_hpcn.dts
@@ -286,6 +286,125 @@
 				f800 0 0 4 &i8259 0 0
 				>;
 
+			pci@00 {
+				device_type = "pci";
+				#interrupt-cells = <1>;
+				#size-cells = <2>;
+				#address-cells = <3>;
+				reg = <0000 0 0 0 0>;
+				bus-range = <0 fe>;
+				ranges = <02000000 0 80000000 80000000 0
+					  20000000
+					  01000000 0 00000000 00000000 0
+					  00100000>;
+
+				pci@00 {
+					device_type = "pci";
+					#interrupt-cells = <1>;
+					#size-cells = <2>;
+					#address-cells = <3>;
+					reg = <0000 0 0 0 0>;
+					bus-range = <1 fe>;
+					ranges = <02000000 0 80000000 80000000
+						  0 20000000
+						  01000000 0 00000000 00000000
+						  0 00100000>;
+					interrupt-map-mask = <3f800 0 0 7>;
+					interrupt-map = <
+						/* IDSEL 0x11 */
+						28800 0 0 1 &i8259 3 2
+						28800 0 0 2 &i8259 4 2
+						28800 0 0 3 &i8259 5 2
+						28800 0 0 4 &i8259 6 2
+
+						/* IDSEL 0x12 */
+						29000 0 0 1 &i8259 4 2
+						29000 0 0 2 &i8259 5 2
+						29000 0 0 3 &i8259 6 2
+						29000 0 0 4 &i8259 3 2
+
+						/* IDSEL 0x13 */
+						29800 0 0 1 &i8259 0 0
+						29800 0 0 2 &i8259 0 0
+						29800 0 0 3 &i8259 0 0
+						29800 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x14 */
+						2a000 0 0 1 &i8259 0 0
+						2a000 0 0 2 &i8259 0 0
+						2a000 0 0 3 &i8259 0 0
+						2a000 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x15 */
+						2a800 0 0 1 &i8259 0 0
+						2a800 0 0 2 &i8259 0 0
+						2a800 0 0 3 &i8259 0 0
+						2a800 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x16 */
+						2b000 0 0 1 &i8259 0 0
+						2b000 0 0 2 &i8259 0 0
+						2b000 0 0 3 &i8259 0 0
+						2b000 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x17 */
+						2b800 0 0 1 &i8259 0 0
+						2b800 0 0 2 &i8259 0 0
+						2b800 0 0 3 &i8259 0 0
+						2b800 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x18 */
+						2c000 0 0 1 &i8259 0 0
+						2c000 0 0 2 &i8259 0 0
+						2c000 0 0 3 &i8259 0 0
+						2c000 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x19 */
+						2c800 0 0 1 &i8259 0 0
+						2c800 0 0 2 &i8259 0 0
+						2c800 0 0 3 &i8259 0 0
+						2c800 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x1a */
+						2d000 0 0 1 &i8259 6 2
+						2d000 0 0 2 &i8259 3 2
+						2d000 0 0 3 &i8259 4 2
+						2d000 0 0 4 &i8259 5 2
+
+						/* IDSEL 0x1b */
+						2d800 0 0 1 &i8259 5 2
+						2d800 0 0 2 &i8259 0 0
+						2d800 0 0 3 &i8259 0 0
+						2d800 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x1c */
+						2e000 0 0 1 &i8259 9 2
+						2e000 0 0 2 &i8259 a 2
+						2e000 0 0 3 &i8259 b 2
+						2e000 0 0 4 &i8259 7 2
+
+						/* IDSEL 0x1d */
+						2e800 0 0 1 &i8259 9 2
+						2e800 0 0 2 &i8259 a 2
+						2e800 0 0 3 &i8259 b 2
+						2e800 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x1e */
+						2f000 0 0 1 &i8259 b 2
+						2f000 0 0 2 &i8259 0 0
+						2f000 0 0 3 &i8259 0 0
+						2f000 0 0 4 &i8259 0 0
+
+						/* IDSEL 0x1f */
+						2f800 0 0 1 &i8259 6 2
+						2f800 0 0 2 &i8259 0 0
+						2f800 0 0 3 &i8259 0 0
+						2f800 0 0 4 &i8259 0 0
+						>;
+					};
+				};
+
+
 			isa@f0 {
 				device_type = "isa";
 				#interrupt-cells = <2>;
@@ -335,7 +454,7 @@
 			#size-cells = <2>;
 			#address-cells = <3>;
 			reg = <9000 1000>;
-			bus-range = <0 ff>;
+			bus-range = <3 ff>;
 			ranges = <02000000 0 a0000000 a0000000 0 20000000
 				  01000000 0 00000000 e3000000 0 00100000>;
 			clock-frequency = <1fca055>;
Index: linux-2.6-8641/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
===================================================================
--- linux-2.6-8641.orig/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
+++ linux-2.6-8641/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
@@ -419,8 +419,12 @@ mpc86xx_hpcn_setup_arch(void)
 	}
 
 #ifdef CONFIG_PCI
-	for (np = NULL; (np = of_find_node_by_type(np, "pci")) != NULL;)
-		add_bridge(np);
+	for (np = NULL; (np = of_find_node_by_type(np, "pci")) != NULL;) {
+		struct device_node *parent = of_get_parent(np);
+		if (strcmp(parent->type, "pci"))
+			add_bridge(np);
+		of_node_put(parent);
+	}
 
 	ppc_md.pci_exclude_device = mpc86xx_exclude_device;
 #endif

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02 hardware

[Jon Loeliger]

On Tue, 2007-05-15 at 11:50, Wade Farnsworth wrote:
> +static void __devinit early_mpc86xx_pcie(struct pci_dev *dev)
> +{
> +	   dev->class = PCI_CLASS_BRIDGE_PCI << 8 | 0x1;
> +}

What'd we decide on that 0x1 in there?

Thanks,
jdl

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02 hardware

[Wade Farnsworth]

On Wed, 2007-05-16 at 11:44 -0500, Jon Loeliger wrote:
> On Tue, 2007-05-15 at 11:50, Wade Farnsworth wrote:
> > +static void __devinit early_mpc86xx_pcie(struct pci_dev *dev)
> > +{
> > +	   dev->class = PCI_CLASS_BRIDGE_PCI << 8 | 0x1;
> > +}
> 
> What'd we decide on that 0x1 in there?
> 
> Thanks,
> jdl
> 
> 

I originally thought that it the 0x1 was necessary, but it was pointed
out to me that it should not be.

If I remove the 0x1 I get some I/O resource allocation failures on the
P2P bridge and its child devices.  I'm currently investigating what the
cause of this is.

--Wade

RE: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev1.02 hardware

[Jin Zhengxiong-R64188]

> -----Original Message-----
> From: linuxppc-dev-bounces+jinzhengxiong=3Dgmail.com@ozlabs.org=20
> [mailto:linuxppc-dev-bounces+jinzhengxiong=3Dgmail.com@ozlabs.or
> g] On Behalf Of Wade Farnsworth
> Sent: Thursday, May 17, 2007 2:07 AM
> To: Loeliger Jon-LOELIGER
> Cc: linuxppc-dev
> Subject: Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0=20
> silicon and Rev1.02 hardware
>=20
> On Wed, 2007-05-16 at 11:44 -0500, Jon Loeliger wrote:
> > On Tue, 2007-05-15 at 11:50, Wade Farnsworth wrote:
> > > +static void __devinit early_mpc86xx_pcie(struct pci_dev *dev) {
> > > +	   dev->class =3D PCI_CLASS_BRIDGE_PCI << 8 | 0x1; }
> >=20
> > What'd we decide on that 0x1 in there?
> >=20
> > Thanks,
> > jdl
> >=20
> >=20
>=20
> I originally thought that it the 0x1 was necessary, but it=20
> was pointed out to me that it should not be.
>=20
> If I remove the 0x1 I get some I/O resource allocation=20
> failures on the P2P bridge and its child devices.  I'm=20
> currently investigating what the cause of this is.

> --Wade
>=20

As you changed the class of the bridge, dev->transparent will be set
again in the later  code, if you remove the 0x01 here, the
dev->transparent will be set to 0 later. So if you changed the class
like this, you do not need to set  dev->transparent =3D 1 in your patch.

Jason=20




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

RE: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev1.02 hardware

[Wade Farnsworth]

On Fri, 2007-05-18 at 14:52 +0800, Jin Zhengxiong-R64188 wrote:
> 
> As you changed the class of the bridge, dev->transparent will be set
> again in the later  code, if you remove the 0x01 here, the
> dev->transparent will be set to 0 later. So if you changed the class
> like this, you do not need to set  dev->transparent = 1 in your patch.
> 
> Jason 

It is my understanding that neither dev->transparent, nor that bit in
the PCI class should be set.

--Wade

RE: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Zhang Wei-r63237]

Hi,=20

If you add below sector to pci@8000, please remove the interrupt-map
sector from pci@8000.
> @@ -286,6 +286,125 @@
>  				f800 0 0 4 &i8259 0 0
>  				>;
> =20
> +			pci@00 {
> +				device_type =3D "pci";
> +				#interrupt-cells =3D <1>;
> +				#size-cells =3D <2>;
> +				#address-cells =3D <3>;
> +				reg =3D <0000 0 0 0 0>;
> +				bus-range =3D <0 fe>;
> +				ranges =3D <02000000 0 80000000 80000000 0
> +					  20000000
> +					  01000000 0 00000000 00000000 0
> +					  00100000>;
> +
> +				pci@00 {
> +					device_type =3D "pci";
> +					#interrupt-cells =3D <1>;
> +					#size-cells =3D <2>;
> +					#address-cells =3D <3>;
> +					reg =3D <0000 0 0 0 0>;
> +					bus-range =3D <1 fe>;
> +					ranges =3D <02000000 0=20
> 80000000 80000000
> +						  0 20000000
> +						  01000000 0=20
> 00000000 00000000
> +						  0 00100000>;
> +					interrupt-map-mask =3D=20
> <3f800 0 0 7>;
> +					interrupt-map =3D <
> +						/* IDSEL 0x11 */
> +						28800 0 0 1 &i8259 3 2
> +						28800 0 0 2 &i8259 4 2
> +						28800 0 0 3 &i8259 5 2
> +						28800 0 0 4 &i8259 6 2
> +
> +						/* IDSEL 0x12 */
> +						29000 0 0 1 &i8259 4 2
> +						29000 0 0 2 &i8259 5 2
> +						29000 0 0 3 &i8259 6 2
> +						29000 0 0 4 &i8259 3 2
> +
> +						/* IDSEL 0x13 */
> +						29800 0 0 1 &i8259 0 0
> +						29800 0 0 2 &i8259 0 0
> +						29800 0 0 3 &i8259 0 0
> +						29800 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x14 */
> +						2a000 0 0 1 &i8259 0 0
> +						2a000 0 0 2 &i8259 0 0
> +						2a000 0 0 3 &i8259 0 0
> +						2a000 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x15 */
> +						2a800 0 0 1 &i8259 0 0
> +						2a800 0 0 2 &i8259 0 0
> +						2a800 0 0 3 &i8259 0 0
> +						2a800 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x16 */
> +						2b000 0 0 1 &i8259 0 0
> +						2b000 0 0 2 &i8259 0 0
> +						2b000 0 0 3 &i8259 0 0
> +						2b000 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x17 */
> +						2b800 0 0 1 &i8259 0 0
> +						2b800 0 0 2 &i8259 0 0
> +						2b800 0 0 3 &i8259 0 0
> +						2b800 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x18 */
> +						2c000 0 0 1 &i8259 0 0
> +						2c000 0 0 2 &i8259 0 0
> +						2c000 0 0 3 &i8259 0 0
> +						2c000 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x19 */
> +						2c800 0 0 1 &i8259 0 0
> +						2c800 0 0 2 &i8259 0 0
> +						2c800 0 0 3 &i8259 0 0
> +						2c800 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x1a */
> +						2d000 0 0 1 &i8259 6 2
> +						2d000 0 0 2 &i8259 3 2
> +						2d000 0 0 3 &i8259 4 2
> +						2d000 0 0 4 &i8259 5 2
> +
> +						/* IDSEL 0x1b */
> +						2d800 0 0 1 &i8259 5 2
> +						2d800 0 0 2 &i8259 0 0
> +						2d800 0 0 3 &i8259 0 0
> +						2d800 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x1c */
> +						2e000 0 0 1 &i8259 9 2
> +						2e000 0 0 2 &i8259 a 2
> +						2e000 0 0 3 &i8259 b 2
> +						2e000 0 0 4 &i8259 7 2
> +
> +						/* IDSEL 0x1d */
> +						2e800 0 0 1 &i8259 9 2
> +						2e800 0 0 2 &i8259 a 2
> +						2e800 0 0 3 &i8259 b 2
> +						2e800 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x1e */
> +						2f000 0 0 1 &i8259 b 2
> +						2f000 0 0 2 &i8259 0 0
> +						2f000 0 0 3 &i8259 0 0
> +						2f000 0 0 4 &i8259 0 0
> +
> +						/* IDSEL 0x1f */
> +						2f800 0 0 1 &i8259 6 2
> +						2f800 0 0 2 &i8259 0 0
> +						2f800 0 0 3 &i8259 0 0
> +						2f800 0 0 4 &i8259 0 0
> +						>;
> +					};
> +				};
> +
> +
>  			isa@f0 {
>  				device_type =3D "isa";
>  				#interrupt-cells =3D <2>;
> @@ -335,7 +454,7 @@
>  			#size-cells =3D <2>;
>  			#address-cells =3D <3>;
>  			reg =3D <9000 1000>;
> -			bus-range =3D <0 ff>;
> +			bus-range =3D <3 ff>;
>  			ranges =3D <02000000 0 a0000000 a0000000=20
> 0 20000000
>  				  01000000 0 00000000 e3000000=20
> 0 00100000>;
>  			clock-frequency =3D <1fca055>;

Best Regards,
Zhang Wei

RE: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Wade Farnsworth]

On Thu, 2007-05-17 at 10:50 +0800, Zhang Wei-r63237 wrote:
> Hi, 
> 
> If you add below sector to pci@8000, please remove the interrupt-map
> sector from pci@8000.

Correct me if I'm wrong, but I think if the interrupt-map for pci@8000
is removed then the pci devices on the revision 1.0 board will not get
the correct interrupts.  I don't currently have a rev 1.0 to test
though.  Can someone with a rev 1.0 HPCN board confirm this?

--Wade

> > @@ -286,6 +286,125 @@
> >  				f800 0 0 4 &i8259 0 0
> >  				>;
> >  
> > +			pci@00 {
> > +				device_type = "pci";
> > +				#interrupt-cells = <1>;
> > +				#size-cells = <2>;
> > +				#address-cells = <3>;
> > +				reg = <0000 0 0 0 0>;
> > +				bus-range = <0 fe>;
> > +				ranges = <02000000 0 80000000 80000000 0
> > +					  20000000
> > +					  01000000 0 00000000 00000000 0
> > +					  00100000>;
> > +
> > +				pci@00 {
> > +					device_type = "pci";
> > +					#interrupt-cells = <1>;
> > +					#size-cells = <2>;
> > +					#address-cells = <3>;
> > +					reg = <0000 0 0 0 0>;
> > +					bus-range = <1 fe>;
> > +					ranges = <02000000 0 
> > 80000000 80000000
> > +						  0 20000000
> > +						  01000000 0 
> > 00000000 00000000
> > +						  0 00100000>;
> > +					interrupt-map-mask = 
> > <3f800 0 0 7>;
> > +					interrupt-map = <
> > +						/* IDSEL 0x11 */
> > +						28800 0 0 1 &i8259 3 2
> > +						28800 0 0 2 &i8259 4 2
> > +						28800 0 0 3 &i8259 5 2
> > +						28800 0 0 4 &i8259 6 2
> > +
> > +						/* IDSEL 0x12 */
> > +						29000 0 0 1 &i8259 4 2
> > +						29000 0 0 2 &i8259 5 2
> > +						29000 0 0 3 &i8259 6 2
> > +						29000 0 0 4 &i8259 3 2
> > +
> > +						/* IDSEL 0x13 */
> > +						29800 0 0 1 &i8259 0 0
> > +						29800 0 0 2 &i8259 0 0
> > +						29800 0 0 3 &i8259 0 0
> > +						29800 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x14 */
> > +						2a000 0 0 1 &i8259 0 0
> > +						2a000 0 0 2 &i8259 0 0
> > +						2a000 0 0 3 &i8259 0 0
> > +						2a000 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x15 */
> > +						2a800 0 0 1 &i8259 0 0
> > +						2a800 0 0 2 &i8259 0 0
> > +						2a800 0 0 3 &i8259 0 0
> > +						2a800 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x16 */
> > +						2b000 0 0 1 &i8259 0 0
> > +						2b000 0 0 2 &i8259 0 0
> > +						2b000 0 0 3 &i8259 0 0
> > +						2b000 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x17 */
> > +						2b800 0 0 1 &i8259 0 0
> > +						2b800 0 0 2 &i8259 0 0
> > +						2b800 0 0 3 &i8259 0 0
> > +						2b800 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x18 */
> > +						2c000 0 0 1 &i8259 0 0
> > +						2c000 0 0 2 &i8259 0 0
> > +						2c000 0 0 3 &i8259 0 0
> > +						2c000 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x19 */
> > +						2c800 0 0 1 &i8259 0 0
> > +						2c800 0 0 2 &i8259 0 0
> > +						2c800 0 0 3 &i8259 0 0
> > +						2c800 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x1a */
> > +						2d000 0 0 1 &i8259 6 2
> > +						2d000 0 0 2 &i8259 3 2
> > +						2d000 0 0 3 &i8259 4 2
> > +						2d000 0 0 4 &i8259 5 2
> > +
> > +						/* IDSEL 0x1b */
> > +						2d800 0 0 1 &i8259 5 2
> > +						2d800 0 0 2 &i8259 0 0
> > +						2d800 0 0 3 &i8259 0 0
> > +						2d800 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x1c */
> > +						2e000 0 0 1 &i8259 9 2
> > +						2e000 0 0 2 &i8259 a 2
> > +						2e000 0 0 3 &i8259 b 2
> > +						2e000 0 0 4 &i8259 7 2
> > +
> > +						/* IDSEL 0x1d */
> > +						2e800 0 0 1 &i8259 9 2
> > +						2e800 0 0 2 &i8259 a 2
> > +						2e800 0 0 3 &i8259 b 2
> > +						2e800 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x1e */
> > +						2f000 0 0 1 &i8259 b 2
> > +						2f000 0 0 2 &i8259 0 0
> > +						2f000 0 0 3 &i8259 0 0
> > +						2f000 0 0 4 &i8259 0 0
> > +
> > +						/* IDSEL 0x1f */
> > +						2f800 0 0 1 &i8259 6 2
> > +						2f800 0 0 2 &i8259 0 0
> > +						2f800 0 0 3 &i8259 0 0
> > +						2f800 0 0 4 &i8259 0 0
> > +						>;
> > +					};
> > +				};
> > +
> > +
> >  			isa@f0 {
> >  				device_type = "isa";
> >  				#interrupt-cells = <2>;
> > @@ -335,7 +454,7 @@
> >  			#size-cells = <2>;
> >  			#address-cells = <3>;
> >  			reg = <9000 1000>;
> > -			bus-range = <0 ff>;
> > +			bus-range = <3 ff>;
> >  			ranges = <02000000 0 a0000000 a0000000 
> > 0 20000000
> >  				  01000000 0 00000000 e3000000 
> > 0 00100000>;
> >  			clock-frequency = <1fca055>;
> 
> Best Regards,
> Zhang Wei

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Kumar Gala]

On May 17, 2007, at 11:03 AM, Wade Farnsworth wrote:

> On Thu, 2007-05-17 at 10:50 +0800, Zhang Wei-r63237 wrote:
>> Hi,
>>
>> If you add below sector to pci@8000, please remove the interrupt-map
>> sector from pci@8000.
>
> Correct me if I'm wrong, but I think if the interrupt-map for pci@8000
> is removed then the pci devices on the revision 1.0 board will not get
> the correct interrupts.  I don't currently have a rev 1.0 to test
> though.  Can someone with a rev 1.0 HPCN board confirm this?

If I'm understanding things this might best be handled by two  
different dts.  Is this because of board changes or chip/silicon  
changes?

- k

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Wade Farnsworth]

On Thu, 2007-05-17 at 11:14 -0500, Kumar Gala wrote:
> On May 17, 2007, at 11:03 AM, Wade Farnsworth wrote:
> 
> > On Thu, 2007-05-17 at 10:50 +0800, Zhang Wei-r63237 wrote:
> >> Hi,
> >>
> >> If you add below sector to pci@8000, please remove the interrupt-map
> >> sector from pci@8000.
> >
> > Correct me if I'm wrong, but I think if the interrupt-map for pci@8000
> > is removed then the pci devices on the revision 1.0 board will not get
> > the correct interrupts.  I don't currently have a rev 1.0 to test
> > though.  Can someone with a rev 1.0 HPCN board confirm this?
> 
> If I'm understanding things this might best be handled by two  
> different dts.  Is this because of board changes or chip/silicon  
> changes?
> 
> - k
> 

Yes.  On rev 1.0 boards, all of the devices on the south bridge are on
bus 0, while on rev 1.02, the devices on the southbridge are on bus 2.

I'd like to use the same dts for both rev's if possible.  But if there
is a reason why they shouldn't, I suppose I could create a separate dts.

--Wade

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Timur Tabi]

Wade Farnsworth wrote:

> Yes.  On rev 1.0 boards, all of the devices on the south bridge are on
> bus 0, while on rev 1.02, the devices on the southbridge are on bus 2.
> 
> I'd like to use the same dts for both rev's if possible.  But if there
> is a reason why they shouldn't, I suppose I could create a separate dts.

I think two DTS files is the best approach for now.  A few of us had an idea to introduce 
conditional statements in to the DTS, and U-Boot would examine the board and/or 
environment variables and then apply the conditions to the device tree before booting the 
kernel.  This would allow you to merge the two DTS files into one, but we're quite a ways 
off from implementing this feature.  In the meantime, two DTS files is okay.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Timur Tabi]

Timur Tabi wrote:

> I think two DTS files is the best approach for now.  A few of us had an 
> idea to introduce conditional statements in to the DTS, and U-Boot would 
> examine the board and/or environment variables and then apply the 
> conditions to the device tree before booting the kernel.  This would 
> allow you to merge the two DTS files into one, but we're quite a ways 
> off from implementing this feature.  In the meantime, two DTS files is 
> okay.

One more thing - I think each DTS file should specify which revision of the board it 
supports, and there should be code in U-Boot and/or Linux to check that revision and 
reject the device tree if there's a mismatch.  We don't want people accidentally using the 
wrong device tree and wondering why it doesn't work.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Scott Wood]

Timur Tabi wrote:
> One more thing - I think each DTS file should specify which revision of the board it 
> supports, and there should be code in U-Boot and/or Linux to check that revision and 
> reject the device tree if there's a mismatch.  We don't want people accidentally using the 
> wrong device tree and wondering why it doesn't work.

We also don't want people using a perfectly good device tree on a newer 
revision of the board that just fixes things and doesn't make any 
device-tree-relevant changes, and wondering why it doesn't work due to a 
failed revision check.

-Scott

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Timur Tabi]

Scott Wood wrote:

> We also don't want people using a perfectly good device tree on a newer 
> revision of the board that just fixes things and doesn't make any 
> device-tree-relevant changes,

Well, either one device tree is okay, or we need two.  If we do need two, then it would be 
bad to load the wrong one on a particular board and have only 90% functionality.  If he 
have the ability to prevent customers from getting confused, then we should do that.  I 
wish this was standard behavior for all device trees.  I've frequently loaded the wrong 
device tree and wondered why nothing worked.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Jerry Van Baren]

Timur Tabi wrote:
> Scott Wood wrote:
> 
>> We also don't want people using a perfectly good device tree on a newer 
>> revision of the board that just fixes things and doesn't make any 
>> device-tree-relevant changes,
> 
> Well, either one device tree is okay, or we need two.  If we do need two, then it would be 
> bad to load the wrong one on a particular board and have only 90% functionality.  If he 
> have the ability to prevent customers from getting confused, then we should do that.  I 
> wish this was standard behavior for all device trees.  I've frequently loaded the wrong 
> device tree and wondered why nothing worked.

Wolfgang Grandegger and Detlev Zundel are working on using fdts for 
configuring u-boot itself (I'm only indirectly involved: I am custodian 
for u-boot-fdt).

I envision the likely progression of this and other activities will be 
to make fdt properties (variables) first class citizens of u-boot: 
substantially and possibly entirely replacing the traditional u-boot 
environment variables.  This would imply that you could write a hush 
script that u-boot executed that would pick the right fdt, or at least 
complain if you had the wrong fdt.

Theoretically.  Some assembly required. :-/

I see Timur just sent an email on the u-boot list proposing a 
"fdt_checkboard()" function.  Hmmm, something to think about...

Best regards,
gvb

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Matt Sealey]

Timur Tabi wrote:
> Scott Wood wrote:
> 
>> We also don't want people using a perfectly good device tree on a newer 
>> revision of the board that just fixes things and doesn't make any 
>> device-tree-relevant changes,
> 
> Well, either one device tree is okay, or we need two.  If we do need two, then it would be 
> bad to load the wrong one on a particular board and have only 90% functionality.  If he 
> have the ability to prevent customers from getting confused, then we should do that.  I 
> wish this was standard behavior for all device trees.  I've frequently loaded the wrong 
> device tree and wondered why nothing worked.

Another design win for dynamically generated firmware device trees and
a knock on the head for writing text files and hoping they match the
hardware description? :D

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Wade Farnsworth]

On Thu, 2007-05-17 at 13:46 -0500, Timur Tabi wrote:
> Wade Farnsworth wrote:
> 
> > Yes.  On rev 1.0 boards, all of the devices on the south bridge are on
> > bus 0, while on rev 1.02, the devices on the southbridge are on bus 2.
> > 
> > I'd like to use the same dts for both rev's if possible.  But if there
> > is a reason why they shouldn't, I suppose I could create a separate dts.
> 
> I think two DTS files is the best approach for now.  A few of us had an idea to introduce 
> conditional statements in to the DTS, and U-Boot would examine the board and/or 
> environment variables and then apply the conditions to the device tree before booting the 
> kernel.  This would allow you to merge the two DTS files into one, but we're quite a ways 
> off from implementing this feature.  In the meantime, two DTS files is okay.
> 

OK.  I'll create a separate dts for the new rev.

--Wade

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[David Gibson]

On Thu, May 17, 2007 at 01:46:26PM -0500, Timur Tabi wrote:
> Wade Farnsworth wrote:
> 
> > Yes.  On rev 1.0 boards, all of the devices on the south bridge are on
> > bus 0, while on rev 1.02, the devices on the southbridge are on bus 2.
> > 
> > I'd like to use the same dts for both rev's if possible.  But if there
> > is a reason why they shouldn't, I suppose I could create a separate dts.
> 
> I think two DTS files is the best approach for now.  A few of us had
> an idea to introduce conditional statements in to the DTS, and

Erm... how would you encode such conditionals in the dtb?  I really
don't like the idea of having a generalized conditional
parser/evaluator built into the bootwrapper.

What I'd been thinking for situations like this is to fold two dtbs
into the bootwrapper and have it select between them based on on board
revision (assuming that can be deduced from registers somehow).

> U-Boot would examine the board and/or environment variables and then
> apply the conditions to the device tree before booting the kernel.
> This would allow you to merge the two DTS files into one, but we're
> quite a ways off from implementing this feature.  In the meantime,
> two DTS files is okay.
> 

-- 
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 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Jerry Van Baren]

David Gibson wrote:
> On Thu, May 17, 2007 at 01:46:26PM -0500, Timur Tabi wrote:
>> Wade Farnsworth wrote:
>>
>>> Yes.  On rev 1.0 boards, all of the devices on the south bridge are on
>>> bus 0, while on rev 1.02, the devices on the southbridge are on bus 2.
>>>
>>> I'd like to use the same dts for both rev's if possible.  But if there
>>> is a reason why they shouldn't, I suppose I could create a separate dts.
>> I think two DTS files is the best approach for now.  A few of us had
>> an idea to introduce conditional statements in to the DTS, and
> 
> Erm... how would you encode such conditionals in the dtb?  I really
> don't like the idea of having a generalized conditional
> parser/evaluator built into the bootwrapper.

I hear forth is a really neat language, and can do conditionals. ;-)

> What I'd been thinking for situations like this is to fold two dtbs
> into the bootwrapper and have it select between them based on on board
> revision (assuming that can be deduced from registers somehow).
> 
>> U-Boot would examine the board and/or environment variables and then
>> apply the conditions to the device tree before booting the kernel.
>> This would allow you to merge the two DTS files into one, but we're
>> quite a ways off from implementing this feature.  In the meantime,
>> two DTS files is okay.

WRT u-boot:

a) With the libfdt changes, we're making good progress on updating the 
fdt based on the board-specific information.  This gives us the 
capability of creating a semi-generic DTS and have u-boot augment the 
fdt with the necessary board-specific property settings.

b) I also have a dream of allowing the hush parser to test fdt 
properties, which would allow us to write u-boot/hush scripts that 
validate that a given fdt is a proper one for the board and/or select 
the proper one out of several in memory.  So many fun ideas, so little 
time...

Best regards,
gvb

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Timur Tabi]

David Gibson wrote:
> On Thu, May 17, 2007 at 01:46:26PM -0500, Timur Tabi wrote:
>> Wade Farnsworth wrote:
>>
>>> Yes.  On rev 1.0 boards, all of the devices on the south bridge are on
>>> bus 0, while on rev 1.02, the devices on the southbridge are on bus 2.
>>>
>>> I'd like to use the same dts for both rev's if possible.  But if there
>>> is a reason why they shouldn't, I suppose I could create a separate dts.
>> I think two DTS files is the best approach for now.  A few of us had
>> an idea to introduce conditional statements in to the DTS, and
> 
> Erm... how would you encode such conditionals in the dtb?  

We'll propose a spec once we have everything figured out.  Our current idea is to allow 
any node or property to have a conditional attached to it.  U-Boot will then scan the 
device tree, evaluate the conditional, and if it's false, delete the particular node/property.

U-Boot will be also be expanded to include the concept of "hardware options", whether the 
user and/or board-specific code can tell U-Boot that hardware option X is set to value Y. 
  The conditions in the device tree will be of the form "X == Y" (or X != Y, X > Y, etc).

For instance, on some board, if jumper 22 is on, then it means that the USB port is 
enabled.  If it's possible for software to scan the status of J22, then the board-specific 
code will do that, and it will create an environment variable "J22=ON".  The USB node in 
the device tree will have the conditional "J22 = ON".

> I really
> don't like the idea of having a generalized conditional
> parser/evaluator built into the bootwrapper.

Well, let us present the full proposal with our reasonings when we're ready.  I don't want 
to engage in speculative debate.

> What I'd been thinking for situations like this is to fold two dtbs
> into the bootwrapper and have it select between them based on on board
> revision (assuming that can be deduced from registers somehow).

Our idea is very similar, but just more standardized and more granular.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Jerry Van Baren]

Timur Tabi wrote:
> David Gibson wrote:
>> On Thu, May 17, 2007 at 01:46:26PM -0500, Timur Tabi wrote:
>>> Wade Farnsworth wrote:
>>>
>>>> Yes.  On rev 1.0 boards, all of the devices on the south bridge are on
>>>> bus 0, while on rev 1.02, the devices on the southbridge are on bus 2.
>>>>
>>>> I'd like to use the same dts for both rev's if possible.  But if there
>>>> is a reason why they shouldn't, I suppose I could create a separate dts.
>>> I think two DTS files is the best approach for now.  A few of us had
>>> an idea to introduce conditional statements in to the DTS, and
>> Erm... how would you encode such conditionals in the dtb?  
> 
> We'll propose a spec once we have everything figured out.  Our current idea is to allow 
> any node or property to have a conditional attached to it.  U-Boot will then scan the 
> device tree, evaluate the conditional, and if it's false, delete the particular node/property.
> 
> U-Boot will be also be expanded to include the concept of "hardware options", whether the 
> user and/or board-specific code can tell U-Boot that hardware option X is set to value Y. 
>   The conditions in the device tree will be of the form "X == Y" (or X != Y, X > Y, etc).
> 
> For instance, on some board, if jumper 22 is on, then it means that the USB port is 
> enabled.  If it's possible for software to scan the status of J22, then the board-specific 
> code will do that, and it will create an environment variable "J22=ON".  The USB node in 
> the device tree will have the conditional "J22 = ON".

I am skeptical.

If hardware can discover configuration details, u-boot software, using 
libfdt, can modify existing and add new configuration items to the fdt 
(modify properties, create nodes, add properties, select the proper fdt 
snippets and merge them into the base fdt).

As I understand it, this is what OF does.  Stuff that isn't probable 
needs to be hardcoded in the base fdt.  Stuff that is configurable 
should be probed and the pieces of fdt generated/selected and added to 
the base fdt.

U-Boot: OF without the RPN.  ;-)

>> I really
>> don't like the idea of having a generalized conditional
>> parser/evaluator built into the bootwrapper.
> 
> Well, let us present the full proposal with our reasonings when we're ready.  I don't want 
> to engage in speculative debate.

Oops, too late. :-D

>> What I'd been thinking for situations like this is to fold two dtbs
>> into the bootwrapper and have it select between them based on on board
>> revision (assuming that can be deduced from registers somehow).
> 
> Our idea is very similar, but just more standardized and more granular.

Best regards,
gvb

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Scott Wood]

On Fri, May 18, 2007 at 09:34:46AM -0500, Timur Tabi wrote:
> We'll propose a spec once we have everything figured out.  Our current
> idea is to allow any node or property to have a conditional attached to
> it.  U-Boot will then scan the device tree, evaluate the conditional,
> and if it's false, delete the particular node/property.
> 
> U-Boot will be also be expanded to include the concept of "hardware
> options", whether the user and/or board-specific code can tell U-Boot
> that hardware option X is set to value Y. The conditions in the device
>   tree will be of the form "X == Y" (or X != Y, X > Y, etc).
> 
> For instance, on some board, if jumper 22 is on, then it means that the
> USB port is enabled.  If it's possible for software to scan the status
> of J22, then the board-specific code will do that, and it will create
> an environment variable "J22=ON".  The USB node in the device tree will
> have the conditional "J22 = ON".

I'd like to point out that what I originally proposed was much simpler
than this; it simply allowed a special section of the device tree to list
jumpers and other hw options, and associate a device tree fragment with
each possibility.  Something like this:

u-boot,hwoptions {
	J1 {
		description = "USB PHY selector";
		
		off {
			description = "USB internal PHY";
			
			tree {
				&usb {
					phy_type = "utmi_wide";
				};
			};
		};

		on {
			description = "USB external PHY";
			
			tree {
				&usb {
					phy_type = "ulpi";
				};
			};
		};
	};
};

The fragment that corresponds to the option that is either detected
automatically or specified by the user on the command line gets merged
into the main dts.  The u-boot,hwoptions tree gets removed before passing
to the kernel, to avoid confusion.  The descriptions can be used to
provide interactive help text, as an alternative to having to fetch the
manual to find jumper information.

I agreed with using conditionals at the dtc level, and having dtc
transform it into the above at the dtb level.  I'm not quite comfortable
with having general conditional expressions at the binary level.

-Scott

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Jerry Van Baren]

Scott Wood wrote:
> On Fri, May 18, 2007 at 09:34:46AM -0500, Timur Tabi wrote:
>> We'll propose a spec once we have everything figured out.  Our current
>> idea is to allow any node or property to have a conditional attached to
>> it.  U-Boot will then scan the device tree, evaluate the conditional,
>> and if it's false, delete the particular node/property.
>>
>> U-Boot will be also be expanded to include the concept of "hardware
>> options", whether the user and/or board-specific code can tell U-Boot
>> that hardware option X is set to value Y. The conditions in the device
>>   tree will be of the form "X == Y" (or X != Y, X > Y, etc).
>>
>> For instance, on some board, if jumper 22 is on, then it means that the
>> USB port is enabled.  If it's possible for software to scan the status
>> of J22, then the board-specific code will do that, and it will create
>> an environment variable "J22=ON".  The USB node in the device tree will
>> have the conditional "J22 = ON".
> 
> I'd like to point out that what I originally proposed was much simpler
> than this; it simply allowed a special section of the device tree to list
> jumpers and other hw options, and associate a device tree fragment with
> each possibility.  Something like this:
> 
> u-boot,hwoptions {
> 	J1 {
> 		description = "USB PHY selector";
> 		
> 		off {
> 			description = "USB internal PHY";
> 			
> 			tree {
> 				&usb {
> 					phy_type = "utmi_wide";
> 				};
> 			};
> 		};
> 
> 		on {
> 			description = "USB external PHY";
> 			
> 			tree {
> 				&usb {
> 					phy_type = "ulpi";
> 				};
> 			};
> 		};
> 	};
> };
> 
> The fragment that corresponds to the option that is either detected
> automatically or specified by the user on the command line gets merged
> into the main dts.  The u-boot,hwoptions tree gets removed before passing
> to the kernel, to avoid confusion.  The descriptions can be used to
> provide interactive help text, as an alternative to having to fetch the
> manual to find jumper information.
> 
> I agreed with using conditionals at the dtc level, and having dtc
> transform it into the above at the dtb level.  I'm not quite comfortable
> with having general conditional expressions at the binary level.
> 
> -Scott

Aye, that looks useful and reasonable to me.  In one of my previous 
spiels I talked about merging fragments of fdt blobs.  That was a 
half-baked thought, the above dts (blob) would have all the fragments in 
it and the u-boot board-specific logic would be copying/moving the 
selected nodes.  That has a lot of benefits...
* The above should result in reasonable board-specific code.
   * We may want to invent a libfdt move/copy subtree function (I don't
       recall one being in there).  Board specific code would want to
       move selected subtrees out of /u-boot,hwoptions into the
       appropriate final node.  All the pieces are in libfdt, just would
       need to be hooked together in a utility subroutine.
* With the above, nothing in the infrastructure (dtc, libfdt) needs to
     change.

On a related note, would it be better to name the node 
"/u-boot/hwoptions" (two levels deep)?  It seems very desirable to me to 
avoid cluttering the root node.  I would like to see, for instance, 
"/u-boot-env" be moved to "/u-boot/env" and probably "/bd_t" moved to 
"/u-boot/bd_t" (if it survives - the fdt should make bd_t obsolete).

Best regards,
gvb

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Timur Tabi]

Jerry Van Baren wrote:

>> I'd like to point out that what I originally proposed was much simpler
>> than this; 

Actually, we're talking about the same thing, but I guess my over-simplification back-fired.

> Aye, that looks useful and reasonable to me.  In one of my previous 
> spiels I talked about merging fragments of fdt blobs.  That was a 
> half-baked thought, the above dts (blob) would have all the fragments in 
> it and the u-boot board-specific logic would be copying/moving the 
> selected nodes.  That has a lot of benefits...

The idea stemmed from a desire to have a single DTS file for a given board, even in 
situations where jumpers changed what hardware was enabled.

> * The above should result in reasonable board-specific code.
>   * We may want to invent a libfdt move/copy subtree function (I don't
>       recall one being in there).  Board specific code would want to
>       move selected subtrees out of /u-boot,hwoptions into the
>       appropriate final node.  All the pieces are in libfdt, just would
>       need to be hooked together in a utility subroutine.

Just being able to delete a given node/property would be enough.  By default, the nodes 
with conditionals would be present in the device tree.  The boot loader would then also 
strip out all conditionals from the device tree before passing it to the kernel.  The 
kernel would then receive a standard device tree like it does today.

> * With the above, nothing in the infrastructure (dtc, libfdt) needs to
>     change.
> 
> On a related note, would it be better to name the node 
> "/u-boot/hwoptions" (two levels deep)?  It seems very desirable to me to 

It's not a u-boot-specific concept.  The idea of representing jumpers (and other hardware 
options) in the device tree is not something that's unique to u-boot or any boot loader. 
The conditionals, however, are a bootloader-specific concept.  We don't want Linux to see 
them.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Scott Wood]

Timur Tabi wrote:
> Just being able to delete a given node/property would be enough. 

I'd rather have an additive model than a subtractive one.  And it would 
not be enough, as you would not be able to have two different versions 
of a property with the same name.

>> On a related note, would it be better to name the node 
>> "/u-boot/hwoptions" (two levels deep)?  It seems very desirable to me to 
> 
> 
> It's not a u-boot-specific concept.  The idea of representing jumpers 
> (and other hardware options) in the device tree is not something that's 
> unique to u-boot or any boot loader. The conditionals, however, are a 
> bootloader-specific concept.  We don't want Linux to see them.

I put the u-boot namespace qualifier on there because the implementation 
is being suggested in the context of u-boot, and it's not a general OF 
binding.  However, as long as it gets nuked before passing it on to the 
kernel, OF/ePAPR/whatever compliance isn't quite as relevant, so plain 
old "hwoptions" should be OK.

-Scott

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Timur Tabi]

Scott Wood wrote:
> Timur Tabi wrote:
>> Just being able to delete a given node/property would be enough. 
> 
> I'd rather have an additive model than a subtractive one.  And it would 
> not be enough, as you would not be able to have two different versions 
> of a property with the same name.

The model would be additive, but the DTB would be subtractive.  After all, the nodes have 
to *exist* in the DTB that's given to U-Boot.  After U-Boot scans the device tree and 
evaluates the conditions, it has to remove the nodes that fail.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Scott Wood]

Timur Tabi wrote:
> Scott Wood wrote:
> 
>> Timur Tabi wrote:
>>
>>> Just being able to delete a given node/property would be enough. 
>>
>>
>> I'd rather have an additive model than a subtractive one.  And it 
>> would not be enough, as you would not be able to have two different 
>> versions of a property with the same name.
> 
> 
> The model would be additive, but the DTB would be subtractive.

I meant additive at the DTB level.

 > After
> all, the nodes have to *exist* in the DTB that's given to U-Boot.

Yes, they exist under the /hwoptions node.

 > After
> U-Boot scans the device tree and evaluates the conditions, it has to 
> remove the nodes that fail.

That's your model.  Mine involves merging a fragment that corresponds to 
an active hwoption, with no complex conditional evaluation or deletion 
of anything from the main tree.

How would you express the alternate setting of phy_type in a subtractive 
model?  You can't have two nodes with the same name and different 
phy_types, and you can't have two phy_type properties in one node.

-Scott

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Timur Tabi]

Scott Wood wrote:

> That's your model.  Mine involves merging a fragment that corresponds to 
> an active hwoption, with no complex conditional evaluation or deletion 
> of anything from the main tree.

I think my model (which is also Jon's, I think) is easier to read and implement.

> How would you express the alternate setting of phy_type in a subtractive 
> model?  You can't have two nodes with the same name and different 
> phy_types, and you can't have two phy_type properties in one node.

The conditionals would have to be correct:


phy-node [ if j22 = on ]
{
	phy-type = 1
}

phy-node [ if j22 = off ]
{
	phy-type = 2
}

Obviously, j22 can be either on or off, but never both.  If the DTS is coded incorrectly, 
then it will have problems, but I think that's a fair trade-off for the easier implementation.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Scott Wood]

Timur Tabi wrote:
> Scott Wood wrote:
> 
>> That's your model.  

Yes.  That was the whole point of my post, to point out my alternative. 
    You were the one that said, "Actually, we're talking about the same 
thing."

>> Mine involves merging a fragment that corresponds 
>> to an active hwoption, with no complex conditional evaluation or 
>> deletion of anything from the main tree.
> 
> 
> I think my model (which is also Jon's, I think) is easier to read 

What does the dtb-level implementation have to do with ease of reading? 
  I said that I didn't mind conditionals at the source level that get 
turned to hwoptions at the dtb level.

> and implement.

I disagree.

> phy-node [ if j22 = on ]
> {
>     phy-type = 1
> }
> 
> phy-node [ if j22 = off ]
> {
>     phy-type = 2
> }

Ah.

So I take it you'll need a new dtb version to implement this?  No such 
revision would be needed for my model.

-Scott

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Jerry Van Baren]

Timur Tabi wrote:
> Scott Wood wrote:
> 
>> That's your model.  Mine involves merging a fragment that corresponds 
>> to an active hwoption, with no complex conditional evaluation or 
>> deletion of anything from the main tree.
> 
> I think my model (which is also Jon's, I think) is easier to read and 
> implement.
> 
>> How would you express the alternate setting of phy_type in a 
>> subtractive model?  You can't have two nodes with the same name and 
>> different phy_types, and you can't have two phy_type properties in one 
>> node.
> 
> The conditionals would have to be correct:
> 
> 
> phy-node [ if j22 = on ]
> {
>     phy-type = 1
> }
> 
> phy-node [ if j22 = off ]
> {
>     phy-type = 2
> }
> 
> Obviously, j22 can be either on or off, but never both.  If the DTS is 
> coded incorrectly, then it will have problems, but I think that's a fair 
> trade-off for the easier implementation.

OK, your syntax implies fairly significant changes to the dtc/dtb/u-boot 
(and possibly linux, depending on what detritus is left after u-boot 
does the selections).  Scott's syntax fits into the current fdt 
definition, tools, and blob-users with the exception that u-boot would 
have board specific selection code added, but that needs to be added 
regardless.

Best regards,
gvb

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Jon Loeliger]

On Fri, 2007-05-18 at 12:56, Timur Tabi wrote:
> Scott Wood wrote:
> 
> > That's your model.  Mine involves merging a fragment that corresponds to 
> > an active hwoption, with no complex conditional evaluation or deletion 
> > of anything from the main tree.
> 
> I think my model (which is also Jon's, I think) is easier to read and implement.

Harumph.

> 
> phy-node [ if j22 = on ]
> {
> 	phy-type = 1
> }
> 
> phy-node [ if j22 = off ]
> {
> 	phy-type = 2
> }
> 
> Obviously, j22 can be either on or off, but never both.  If the DTS is coded incorrectly, 
> then it will have problems, but I think that's a fair trade-off for the easier implementation.

First of all, I wanted to get away from the notion of calling
anything a "jumper".  What I said to you was to predicate the
clause based on some arbitrary conditional, not just some "jumper
setting."

Second, I never came anywhere near the syntax above.

Third, I am pretty sure we've always really been talking
about a DTC enhancement to selectively add regions to the
DTB in much the same way as if one had said:

    #if <compile-time-conditional>
        {
            <use this node or property snippet>
        }
    #else
        {
            <use this other snippet>

        }
    #endif

So embedding a full runtime evaluation of conditionals
expressions wasn't really in my suggestion at this time
at all.  I had essentially proposed (to you, Timur) that
we have a predicate clause in the DTS that guarded a the
presence/absence of a node.

And finally, I wasn't sure I was happy with it all yet
in any event, so I hadn't acted on it yet.  I was still
pondering "the right approach here".

Ah well,
jdl

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Scott Wood]

Jon Loeliger wrote:
> First of all, I wanted to get away from the notion of calling
> anything a "jumper".  What I said to you was to predicate the
> clause based on some arbitrary conditional, not just some "jumper
> setting."

Yes, I like the "hwoption" name better.

> Second, I never came anywhere near the syntax above.
> 
> Third, I am pretty sure we've always really been talking
> about a DTC enhancement to selectively add regions to the
> DTB in much the same way as if one had said:
> 
>     #if <compile-time-conditional>
>         {
>             <use this node or property snippet>
>         }
>     #else
>         {
>             <use this other snippet>
> 
>         }
>     #endif

No, we were pretty much always talking about a run-time thing.

Compile-time would just give you a more convenient way of generating 
multiple dtbs; it wouldn't address the root problem of combinatorial 
explosion with several independent options.  Even when the number of 
alternatives isn't high, it's much nicer to the user to be able to just 
type a command into u-boot (or better yet, have it be autodetected) when 
an option changes than to have to rebuild and install a new dtb.

-Scott

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Jon Loeliger]

On Fri, 2007-05-18 at 13:23, Scott Wood wrote:
> Jon Loeliger wrote:
> > First of all, I wanted to get away from the notion of calling
> > anything a "jumper".  What I said to you was to predicate the
> > clause based on some arbitrary conditional, not just some "jumper
> > setting."
> 
> Yes, I like the "hwoption" name better.

I think my point was missed.  It could be more general
than "a hardware thing".  Sure, HW contributes.


> No, we were pretty much always talking about a run-time thing.

On he contrary, here are copies of two pieces of mail from
roughly May 2006 and Feb 2007 in which CPP and M4 as _compiled_time_
processing are being discussed.  In the latter, you discuss it more:.

	From: 	Kumar Gala <galak@kernel.crashing.org>
	To: 	Jon Loeliger <jdl@freescale.com>
	Cc:	Jon Loeliger <jdl@jdl.com>, Benjamin Herrenschmidt
		<benh@kernel.crashing.org>, linuxppc-dev@ozlabs.org list
		<linuxppc-dev@ozlabs.org>
	Subject: 	Re: DTC/dts modifications
	Date: 	Mon, 1 May 2006 14:52:23 -0500	

	[snip]


	>> If these used some other symbol instead of '#' cpp 
	>> will be happy and we can use it to create macros for us.
	>
	> Yeah, we're not going to be able to change those; they
	> are "By The Book".

	By what book?  It would seem to me that BNF for dtc is
	completely under our control and if we want to change it we can.
	I understand that there is some correspondence to Open Firmware,
	but it seems that if its people are ok with the dts format
	changing that's a lot easier than implementing tons of support
	in dtc for features that cpp gives us.

	[I'm also guessing no one's really got time to go and implement
	these features in dtc]

	> Instead, we'll have to make the lexical analysis conscious
	> of something like a <newline> context sensitive token or so.
	> Or throw some flag to cpp to not emit location markers.

	- kumar





	From: 	Jon Loeliger <jdl@jdl.com>
	To: 	Benjamin Herrenschmidt <benh@kernel.crashing.org>
	Cc: 	linuxppc-dev@ozlabs.org,
		Roland Dreier <rdreier@cisco.com>,
	          Stefan Roese <sr@denx.de>, linuxppc-embedded@ozlabs.org
	Subject:Re: [PATCH] ppc: Add support for AMCC Taishan 440GX eval board
	Date: 	Mon, 12 Feb 2007 14:16:21 -0600	

	So, like, the other day Benjamin Herrenschmidt mumbled:
	> Note that there are still things that we might want to
	> change. For example, I think we really should look into adding
	> a macro mecanism and/or an include mecanism to dtc so that we
	> can do things like #include <ibm440gp.dtc> to get the base
	> processor/SoC definition and then "overlay" some properties on
	> top of it (like emac phy mode etc...)

	What do people prefer here?  Straight CPP pre-run?
	Direct support built into dtc to do file-inclusion, macros?

	Thoughts, opinions, suggestions, pre-NACKs, Cabernet Franc bribes?

	jdl


And you reply:


	Simple textual macros would make it difficult to define 123A and
	123B SoCs whose device tree nodes are mostly a generic 123, but
	require a few changes in various parts.  I'd rather see dtc
	support overlaying trees, with the "newer" tree able to add,
	modify, and remove nodes and properties from the "older", more
	generic tree.

	Parametric macros (or "template" nodes) might be nice for a few
	things on top of that, though (preferably with better syntax
	than CPP).

Clearly, we have in fact historically discussed compile-time
conditional DTC behavior.

> Compile-time would just give you a more convenient way of generating 
> multiple dtbs; it wouldn't address the root problem of combinatorial 
> explosion with several independent options.  Even when the number of 
> alternatives isn't high, it's much nicer to the user to be able to just 
> type a command into u-boot (or better yet, have it be autodetected) when 
> an option changes than to have to rebuild and install a new dtb.

Yes, I understand.  But I think, say, the Linux build handles
the compile time combinatorial explosion of compile time options
reasonably well so far... :-)

jdl

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Scott Wood]

Jon Loeliger wrote:
> On Fri, 2007-05-18 at 13:23, Scott Wood wrote:
> 
>>Jon Loeliger wrote:
>>
>>>First of all, I wanted to get away from the notion of calling
>>>anything a "jumper".  What I said to you was to predicate the
>>>clause based on some arbitrary conditional, not just some "jumper
>>>setting."
>>
>>Yes, I like the "hwoption" name better.
> 
> 
> I think my point was missed.  It could be more general
> than "a hardware thing".  Sure, HW contributes.

What, other than hardware differences, would one conditionalize on?

>>No, we were pretty much always talking about a run-time thing.
> 
> 
> On he contrary, here are copies of two pieces of mail from
> roughly May 2006 and Feb 2007 in which CPP and M4 as _compiled_time_
> processing are being discussed.  In the latter, you discuss it more:.

That was a separate discussion for a separate purpose (reducing 
redundancy and duplication among dts files).  I was talking about the 
discussions I had with Timur about a month ago, to which you responded 
with the conditional suggestion.

-Scott

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Timur Tabi]

Jon Loeliger wrote:

>> I think my model (which is also Jon's, I think) is easier to read and implement.
> 
> Harumph.

Hey, I got the idea from you!

> First of all, I wanted to get away from the notion of calling
> anything a "jumper".  What I said to you was to predicate the
> clause based on some arbitrary conditional, not just some "jumper
> setting."

True, but the arbitrary condition would, in practice, be the value of some 
jum^H^H^Hhardware setting.

> Second, I never came anywhere near the syntax above.

That's just a rough version because I don't have the DTS layout memorized.

> Third, I am pretty sure we've always really been talking
> about a DTC enhancement to selectively add regions to the
> DTB in much the same way as if one had said:

The problem is that the conditions that I'm talking about are not known until U-Boot runs. 
  If your idea was only compile-time conditional, then I'm just extending it to a runtime 
conditional.

> So embedding a full runtime evaluation of conditionals
> expressions wasn't really in my suggestion at this time
> at all.  I had essentially proposed (to you, Timur) that
> we have a predicate clause in the DTS that guarded a the
> presence/absence of a node.

predicate clause == conditional.

> And finally, I wasn't sure I was happy with it all yet
> in any event, so I hadn't acted on it yet.  I was still
> pondering "the right approach here".

I was trying to avoid a premature discussion on this idea, but apparently everyone else 
wants to talk about it!

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev 1.02hardware

[Jerry Van Baren]

Timur Tabi wrote:
> Jerry Van Baren wrote:

[snip]

>> * The above should result in reasonable board-specific code.
>>   * We may want to invent a libfdt move/copy subtree function (I don't
>>       recall one being in there).  Board specific code would want to
>>       move selected subtrees out of /u-boot,hwoptions into the
>>       appropriate final node.  All the pieces are in libfdt, just would
>>       need to be hooked together in a utility subroutine.
> 
> Just being able to delete a given node/property would be enough.  By 
> default, the nodes with conditionals would be present in the device 
> tree.  The boot loader would then also strip out all conditionals from 
> the device tree before passing it to the kernel.  The kernel would then 
> receive a standard device tree like it does today.

Deleting isn't really enough because you would want to copy/move the 
configuration from a subnode of the hwoptions node to the final resting 
place in the standard tree (assuming there is a final resting place in 
the standard tree - I'm picturing SOC configuration choices here). 
Simple deletion isn't adequate in general because
* It would be silly to change linux to look in a new location for things
     that already exist in standard locations (e.g. SOC stuff)
* Deleting a node inherently deletes all of the subnodes.  In order to
     do only delete operations, you would end up with an ugly path like:
/u-boot,hwoptions/J1/on/tree/&usb/phy_type (set to  "ulpi")
     where I suspect the &usb/phy_type portion should be glued into a SOC
     (or other) pre-existing subnode.
* It is possible some choices can be used multiple times - the above
     example specifies a PHY.  There are very often multiple instances of
     only type of PHY and copying one "prototype" selection into multiple
     final destination nodes could save space and effort.

>> * With the above, nothing in the infrastructure (dtc, libfdt) needs to
>>     change.
>>
>> On a related note, would it be better to name the node 
>> "/u-boot/hwoptions" (two levels deep)?  It seems very desirable to me to 
> 
> It's not a u-boot-specific concept.  The idea of representing jumpers 
> (and other hardware options) in the device tree is not something that's 
> unique to u-boot or any boot loader. The conditionals, however, are a 
> bootloader-specific concept.  We don't want Linux to see them.

Well, then we shouldn't name it u-boot,hwoptions.  Ahh, nevermind, Scott 
beat me to that point.

Best regards,
gvb

RE: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev1.02hardware

[Zhang Wei-r63237]

For support rev 1.0 boards, you can only remove the
pci@00:pci@00:interrupt-map sector. The pci driver can use
pci@8000:interrupt-map for irq mapping.

Such as below:

diff --git a/arch/powerpc/boot/dts/mpc8641_hpcn.dts
b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
index 260b264..04626b1 100644
--- a/arch/powerpc/boot/dts/mpc8641_hpcn.dts
+++ b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
@@ -297,6 +297,12 @@
 				interrupts =3D <49 2>;
 				interrupt-parent =3D <&mpic>;
 			};
+			uli1575: uli1575@100 {
+				reg =3D <0 0 0 0 0>;
+				pci_bridge@200 {
+					reg =3D <0 0 0 0 0>;
+				};
+			};
=20
 		};

Cheers,
Wei
> > On May 17, 2007, at 11:03 AM, Wade Farnsworth wrote:
> >=20
> > > On Thu, 2007-05-17 at 10:50 +0800, Zhang Wei-r63237 wrote:
> > >> Hi,
> > >>
> > >> If you add below sector to pci@8000, please remove the=20
> interrupt-map
> > >> sector from pci@8000.
> > >
> > > Correct me if I'm wrong, but I think if the interrupt-map=20
> for pci@8000
> > > is removed then the pci devices on the revision 1.0 board=20
> will not get
> > > the correct interrupts.  I don't currently have a rev 1.0 to test
> > > though.  Can someone with a rev 1.0 HPCN board confirm this?
> >=20
> > If I'm understanding things this might best be handled by two =20
> > different dts.  Is this because of board changes or chip/silicon =20
> > changes?
> >=20
> > - k
> >=20
>=20
> Yes.  On rev 1.0 boards, all of the devices on the south bridge are on
> bus 0, while on rev 1.02, the devices on the southbridge are on bus 2.
>=20
> I'd like to use the same dts for both rev's if possible.  But if there
> is a reason why they shouldn't, I suppose I could create a=20
> separate dts.
>=20
> --Wade
>=20
>=20

RE: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon and Rev1.02hardware

[Wade Farnsworth]

On Fri, 2007-05-18 at 18:20 +0800, Zhang Wei-r63237 wrote:
> For support rev 1.0 boards, you can only remove the
> pci@00:pci@00:interrupt-map sector. The pci driver can use
> pci@8000:interrupt-map for irq mapping.
> 
> Such as below:
> 
> diff --git a/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> index 260b264..04626b1 100644
> --- a/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> +++ b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> @@ -297,6 +297,12 @@
>  				interrupts = <49 2>;
>  				interrupt-parent = <&mpic>;
>  			};
> +			uli1575: uli1575@100 {
> +				reg = <0 0 0 0 0>;
> +				pci_bridge@200 {
> +					reg = <0 0 0 0 0>;
> +				};
> +			};
>  
>  		};
> 
> Cheers,
> Wei

Hmm.  For some reason I thought that wouldn't work, but after some
testing it appears I was wrong.  I'll incorporate that into the next
version of the patchset.

Thanks.

--Wade

RE: [PATCH 5/5] PCI fixes for the MPC8641 Rev 2.0 silicon andRev1.02hardware

[Zhang Wei-r63237]

Hi, Wade,

That's a good news. These codes is one of our patches. I'll commit them
myself.

Thanks!=20

-Wei

>=20
> Hmm.  For some reason I thought that wouldn't work, but after some
> testing it appears I was wrong.  I'll incorporate that into the next
> version of the patchset.
>=20
> Thanks.
>=20
> --Wade
>=20
>=20