Re: [PATCH] [8xx] powerpc: Add support for the MPC852 based mgsuvd board from keymile.

[Scott Wood <scottwood@freescale.com>]

On Thu, Jun 12, 2008 at 04:32:47AM +0400, Vitaly Bordug wrote:
> +			interrupts = <0x0f 2>;		/* decrementer interrupt */

Interrupts should probably be decimal.

> +			compatible = "fsl,cpm1";

compatible = "fsl,mpc852-cpm", "fsl,cpm1".

> +			command-proc = <0x9c0>;

This is obsolete; there should be no remaining code that refers to it.

> +			muram@2000 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				ranges = <0 0x2000 0x2000>;
> +
> +				data@0 {
> +					compatible = "fsl,cpm-muram-data";
> +					reg = <0x800 0x1800>;
> +				};
> +			};

The muram node itself should also have a compatible (fsl,cpm-muram).

> +				clock-frequency = <0x3ef1480>;

clock-frequency should be decimal (or better, filled in by u-boot).

> +			CPM_PIC: interrupt-controller@930 {
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +				interrupts = <5 2 0 2 0x20 2>;
> +				interrupt-parent = <&PIC>;
> +				reg = <0x930 0x20>;
> +				compatible = "fsl,cpm1-pic";

"fsl,mpc852-cpm-pic", "fsl,cpm1-pic".
Likewise elsewhere.

> +			/* MON-1 */
> +			serial@a80 {
> +				device_type = "serial";
> +				compatible = "cpm_uart",
> +				             "fsl,cpm1-smc-uart";
> +				model = "SMC";
> +				device-id = <1>;
> +				reg = <0xa80 0x10 0x3fc0 0x40>;
> +				interrupts = <4>;
> +				interrupt-parent = <&CPM_PIC>;
> +				fsl,cpm-brg = <1>;
> +				fsl,cpm-command = <0x0090>;
> +				current-speed = <0>; /* Filled in by u-boot */
> +			};
> +
> +			ethernet@a40 {
> +				device_type = "network";
> +				compatible = "fs_enet",
> +				             "fsl,cpm1-scc-enet";
> +				model = "SCC";
> +				device-id = <3>;

The "cpm_uart" and "fs_enet" compatible values, as well as the device-id
property and this usage of the model property, are obsolete and there should
be no code remaining that references it.

> +				reg = <0xa40 0x18 0x3e00 0xb0>;
> +				mac-address = [ 00 00 00 00 00 00 ];  /* Filled in by u-boot */

Should be local-mac-address.

> +				fixed-link = <0 0 0x0a 0 0>;

We should move the documentation for this out of the gianfar-specific
section of booting-without-of.txt.  Speed should be decimal.

> +config PPC_MGSUVD
> +	bool "MGSUVD"
> +	select CPM1
> +	select PPC_CPM_NEW_BINDING

No need to explicitly select PPC_CPM_NEW_BINDING any more.

> +struct cpm_pin {
> +		int port, pin, flags;
> +};

Only one tab, please.

> +static __initdata struct of_device_id of_bus_ids[] = {
> +	{ .name = "soc", },
> +	{ .name = "cpm", },
> +	{ .name = "localbus", },
> +	{},
> +};

Replace these with { .compatible = "simple-bus" }, and add simple-bus to the
dts where appropriate.

> +static int __init declare_of_platform_devices(void)
> +{
> +	of_platform_bus_probe (NULL, of_bus_ids, NULL);
> +	return 0;
> +}
> +machine_device_initcall(mgsuvd, declare_of_platform_devices);
> +
> +static int __init mgsuvd_probe(void)
> +{
> +	unsigned long root = of_get_flat_dt_root ();
> +	return of_flat_dt_is_compatible (root, "keymile,mgsuvd");
> +}

No space between a function name and the arguments -- this isn't u-boot. :-)

> +define_machine(mgsuvd) {
> +	.name			= "MGSUVD",
> +	.probe			= mgsuvd_probe,
> +	.setup_arch		= mgsuvd_setup_arch,
> +	.init_IRQ		= mpc8xx_pics_init,
> +	.get_irq		= mpc8xx_get_irq,
> +	.restart		= mpc8xx_restart,
> +	.calibrate_decr		= mpc8xx_calibrate_decr,
> +	.set_rtc_time		= mpc8xx_set_rtc_time,
> +	.get_rtc_time		= mpc8xx_get_rtc_time,
> +};

This tab-alignment will look like hell if viewed with another tab size...

-Scott