Re: [PATCH] [POWERPC] 85xx: Add basic Uniprocessor MPC8572 DS port

[Kumar Gala <galak@kernel.crashing.org>]

On Sep 11, 2007, at 9:17 AM, Scott Wood wrote:

> On Tue, Sep 11, 2007 at 01:29:18AM -0500, Kumar Gala wrote:
>> diff --git a/arch/powerpc/boot/dts/mpc8572ds.dts b/arch/powerpc/ 
>> boot/dts/mpc8572ds.dts
>> new file mode 100644
>> index 0000000..9d23561
>> --- /dev/null
>> +++ b/arch/powerpc/boot/dts/mpc8572ds.dts
>> @@ -0,0 +1,378 @@
>> +/*
>> + * MPC8572 DS Device Tree Source
>> + *
>> + * Copyright 2007 Freescale Semiconductor Inc.
>> + *
>> + * This program is free software; you can redistribute  it and/or  
>> modify it
>> + * under  the terms of  the GNU General  Public License as  
>> published by the
>> + * Free Software Foundation;  either version 2 of the  License,  
>> or (at your
>> + * option) any later version.
>> + */
>> +
>> +/ {
>> +	model = "MPC8572DS";
>> +	compatible = "MPC8572DS", "MPC85xxDS";
>
> "fsl," prefix on compatible.
>
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +
>> +	cpus {
>> +		#cpus = <1>;
>
> Where is #cpus defined or used?  I don't see it used in any other  
> trees, either.

will kill.

>
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		PowerPC,8572@0 {
>> +			device_type = "cpu";
>> +			reg = <0>;
>> +			d-cache-line-size = <20>;	// 32 bytes
>> +			i-cache-line-size = <20>;	// 32 bytes
>> +			d-cache-size = <8000>;		// L1, 32K
>> +			i-cache-size = <8000>;		// L1, 32K
>> +			timebase-frequency = <0>;
>> +			bus-frequency = <0>;
>> +			clock-frequency = <0>;
>> +			32-bit;
>
> Where is 32-bit; defined or used?

its a copy paste error.  Will kill 32-bit on other platforms.

>
>> +	soc8572@ffe00000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		#interrupt-cells = <2>;
>> +		device_type = "soc";
>> +		ranges = <00000000 ffe00000 00100000
>> +			  80000000 80000000 20000000
>> +			  a0000000 a0000000 20000000
>> +			  c0000000 c0000000 20000000
>> +			  ffc00000 ffc00000 00010000
>> +			  ffc10000 ffc10000 00010000
>> +			  ffc20000 ffc20000 00010000>;
>> +
>> +		reg = <ffe00000 00001000>;	// CCSRBAR 1M
>
> Comment doesn't match what's actually in reg.

will fix.

>
>> +		bus-frequency = <0>;		// Filled out by uboot.
>> +
>> +		memory-controller@2000 {
>> +			compatible = "fsl,8572-memory-controller";
>
> Is it compatible with any other 85xx memory controller?

maybe, but I don't want to get into that just yet.

>
>> +			reg = <2000 1000>;
>> +			interrupt-parent = <&mpic>;
>> +			interrupts = <12 2>;
>> +		};
>> +
>> +		l2-cache-controller@20000 {
>> +			compatible = "fsl,8572-l2-cache-controller";
>> +			reg = <20000 1000>;
>> +			cache-line-size = <20>;	// 32 bytes
>> +			cache-size = <80000>;	// L2, 512K
>> +			interrupt-parent = <&mpic>;
>> +			interrupts = <10 2>;
>> +		};
>
> Should this node be referenced by an l2-cache property in the cpu  
> node?

No, its a front side cache.

>
>> +		pcie@8000 {
>> +			compatible = "fsl,mpc8641-pcie";
>
> Should probably be "fsl,mpc8572-pcie", "fsl,mpc8641-pcie".

this should "fsl,mpc8548-pcie" to match other 85xx pcie.

>
>> +			device_type = "pci";
>> +			#interrupt-cells = <1>;
>> +			#size-cells = <2>;
>> +			#address-cells = <3>;
>> +			reg = <8000 1000>;
>> +			bus-range = <0 ff>;
>> +			ranges = <02000000 0 80000000 80000000 0 20000000
>> +				  01000000 0 00000000 ffc00000 0 00010000>;
>
> No prefetchable mem space?

we haven't normally provided prefetch on 85xx/86xx.. will deal with  
this later.

>
>> +			clock-frequency = <1fca055>;
>
> Decimal would be nicer.
>
>> +		pcie@9000 {
>> +			compatible = "fsl,mpc8548-pcie";
>
> Why is this one 8548-compatible and the previous one 8641-compatible?

copy paste, will fix.

>
>> +		mpic: pic@40000 {
>> +			clock-frequency = <0>;
>> +			interrupt-controller;
>> +			#address-cells = <0>;
>> +			#interrupt-cells = <2>;
>> +			reg = <40000 40000>;
>> +			built-in;
>> +			compatible = "chrp,open-pic";
>> +			device_type = "open-pic";
>> +			big-endian;
>> +		};
>
> Where is the built-in; property defined or used?

defined in chrp OF spec, not sure why we bother with it.

>
>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>> index 06d23e1..c98b867 100644
>> --- a/include/linux/pci_ids.h
>> +++ b/include/linux/pci_ids.h
>> @@ -374,10 +374,9 @@
>>  #define PCI_DEVICE_ID_ATI_IXP400_SATA   0x4379
>>  #define PCI_DEVICE_ID_ATI_IXP400_SATA2	0x437a
>>  #define PCI_DEVICE_ID_ATI_IXP600_SATA	0x4380
>> -#define PCI_DEVICE_ID_ATI_IXP600_SMBUS	0x4385
>> +#define PCI_DEVICE_ID_ATI_SBX00_SMBUS	0x4385
>>  #define PCI_DEVICE_ID_ATI_IXP600_IDE	0x438c
>>  #define PCI_DEVICE_ID_ATI_IXP700_SATA	0x4390
>> -#define PCI_DEVICE_ID_ATI_IXP700_SMBUS	0x4395
>>  #define PCI_DEVICE_ID_ATI_IXP700_IDE	0x439c
>
> What's going on here?

copied file over from an older tree.

- k