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

[Scott Wood <scottwood@freescale.com>]

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.

> +		#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?

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

> +		bus-frequency = <0>;		// Filled out by uboot.
> +
> +		memory-controller@2000 {
> +			compatible = "fsl,8572-memory-controller";

Is it compatible with any other 85xx memory controller?

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

> +		pcie@8000 {
> +			compatible = "fsl,mpc8641-pcie";

Should probably be "fsl,mpc8572-pcie", "fsl,mpc8641-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?

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

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

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

-Scott