[PATCH] [POWERPC] 83xx: Add support for the Thecus N1200 NAS device

[Byron Bradley]

On Fri, 5 Jun 2009, David Gibson wrote:

> On Thu, Jun 04, 2009 at 09:59:04PM +0100, Byron Bradley wrote:
> > The Thecus N1200 is a NAS device with a single internal SATA disk and
> > an eSATA port based on an MPC8347 SoC.
> 
> Comments on a number of fairly minor device tree nits below:

Hi David, Peter. Thanks for the comments, replies inline below.

> 
> [snip]
> > +	soc8349 at e0000000 {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		device_type = "soc";
> > +		compatible = "simple-bus";
> 
> The compatible value should have something more specific
> (e.g. "fsl,mpc8340-soc") before listing "simple-bus".

Added "fsl,mpc8347-soc" and changed soc8349 to soc8347.

> 
> > +		ranges = <0x0 0xe0000000 0x00100000
> > +			  0xfe000000 0xfe000000 0x0800000>;
> > +		reg = <0xe0000000 0x00000200>;
> > +		bus-frequency = <0>;			// from bootloader
> > +
> > +		physmap-flash at fe000000 {
> 
> Calling this just "flash" would be more normal.

Done.

> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			compatible = "cfi-flash";
> 
> Ideally this should list the actual model of flash chip, before the
> generic "cfi-flash".

Added "numonyx,28f640j3d"

> > +			reg = <0xfe000000 0x0800000>;
> > +			bank-width = <2>;
> > +			device-width = <1>;
> > +
> > +			u-boot at 0 {
> > +				reg = <0x0 0x040000>;
> > +				read-only;
> > +			};
> > +
> > +			u-boot-config at 40000 {
> > +				reg = <0x040000 0x040000>;
> > +				label = "u-boot config";
> > +			};
> > +
> > +			user at 080000 {
> > +				reg = <0x080000 0x100000>;
> > +			};
> > +
> > +			kernel at 180000 {
> > +				reg = <0x180000 0x1a0000>;
> > +			};
> > +
> > +			ramdisk at 320000 {
> > +				reg = <0x320000 0x4e0000>;
> > +			};
> > +		};
> > +
> > +		wdt at 200 {
> > +			device_type = "watchdog";
> 
> No device_type here.

Removed all device_type entries that you said shouldn't be there.

> > +			fanctrl at 32 {
> 
> A less abbreviated name, like "fan-control" would be more usual here.
> 
> > +				compatible = "fintek,f75375";
> > +				reg = <0x2e>;
> > +			};
> > +
> > +			rtc at 32 {
> > +				device_type = "rtc";
> > +				compatible = "ricoh,rs5c372a";
> > +				reg = <0x32>;
> > +			};
> > +
> > +			boardctrl at 32 {
> 
> Again "board-control" would be preferable.

Both changed.

> > +		ipic: pic at 700 {
> > +			interrupt-controller;
> > +			#address-cells = <0>;
> > +			#interrupt-cells = <2>;
> > +			reg = <0x700 0x100>;
> > +			device_type = "ipic";
> 
> This should have a compatible property.  It shouldn't really have
> device_type, but I suspect that's a bug in the ipic binding, rather
> than your tree per-se.

The binding for this seems to be done in the setup file, 
arch/powerpc/platforms/83xx/thecus_n1200.c in my case. At the moment it 
does:
	np = of_find_node_by_type(NULL, "ipic");
	...
	ipic_init(np, 0);
so making it look for something like "fsl,mpc8347-ipic" should be no 
problem if that's how it should be done.

> > +		};
> > +
> > +		gpio1: gpio-controller at c00 {
> > +			#gpio-cells = <2>;
> > +			compatible = "fsl,mpc8347-gpio", "fsl,mpc8349-gpio";
> 
> This actually is an 8349 board, yes?  Generally compatible should be
> listed from most specific to least specific, so the 8349 entry should
> go first.

No, as Peter pointed out this is an 8347. The soc8349 at the top was 
probably a combination of basing this dts on an 8349 one and the fact that 
most of the freescale docs for 8347 just point to 8349 ones. I've made 
sure the only place 8349 is referenced is in compatible fields and it's 
always after the 8347 version.

Cheers,

-- 
Byron Bradley